Skip to content

ci: fix manual review app deployment instructions#808

Merged
bolinocroustibat merged 6 commits into
mainfrom
fix-review-app-ci
Jul 23, 2025
Merged

ci: fix manual review app deployment instructions#808
bolinocroustibat merged 6 commits into
mainfrom
fix-review-app-ci

Conversation

@bolinocroustibat

@bolinocroustibat bolinocroustibat commented Jul 22, 2025

Copy link
Copy Markdown
Contributor

Closes #807

@abulte abulte left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rather like the pattern with the PR number in the URL, and we've been using it for two years. Plus, this will cause mayhem with branch names like fix/xxx.

If we really want to avoid the PR number as input, we could try to get it from the GitHub API? I did it in reverse here https://github.com/opendatateam/udata-front-kit/pull/802/files#diff-3daa2cb95537798352dc8a58b25d016bdacee39718012369b677c0f04a99aa70R35.

Finally, we may not need to overoptimize the inputs in this workflow since #802 will let us deploy only with the site id.

@bolinocroustibat

Copy link
Copy Markdown
Contributor Author

I rather like the pattern with the PR number in the URL, and we've been using it for two years. Plus, this will cause mayhem with branch names like fix/xxx.

If we really want to avoid the PR number as input, we could try to get it from the GitHub API? I did it in reverse here https://github.com/opendatateam/udata-front-kit/pull/802/files#diff-3daa2cb95537798352dc8a58b25d016bdacee39718012369b677c0f04a99aa70R35.

Finally, we may not need to overoptimize the inputs in this workflow since #802 will let us deploy only with the site id.

Sure, we can find a way to manage that properly from the PR number

@bolinocroustibat bolinocroustibat marked this pull request as ready for review July 23, 2025 08:21
@bolinocroustibat bolinocroustibat self-assigned this Jul 23, 2025
@bolinocroustibat bolinocroustibat requested a review from abulte July 23, 2025 08:55
@bolinocroustibat bolinocroustibat changed the title ci: fix manual deployment workflow ci: fix manual review app deployment workflow Jul 23, 2025

@abulte abulte left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't remove the branch from the input UI on GitHub, right? Feels weird to ignore a native parameter that works as designed... Like I said previously, this feels a bit over optimized and #802 will simplify the workflow anyway.

Comment thread .github/workflows/review-app.yml Outdated
Comment thread .github/workflows/review-app.yml Outdated
@JeSuisUnCaillou JeSuisUnCaillou removed their request for review July 23, 2025 09:19
@bolinocroustibat

bolinocroustibat commented Jul 23, 2025

Copy link
Copy Markdown
Contributor Author

This won't remove the branch from the input UI on GitHub, right? Feels weird to ignore a native parameter that works as designed... Like I said previously, this feels a bit over optimized and #802 will simplify the workflow anyway.

Yes it seems the branch selector in the input UI on GitHub is native and can't be removed.
From what I understood from your previous message I thought you were suggesting to keep the PR selector, sorry from misunderstanding.
So we want no PR input in the GUI, only the native branch one (I agree), but the PR number deduced from the branch? I would think we can deduce a branch from a PR number, but I wouldn't be sure about the opposite, as there can be several PRs from the same branch?

@abulte

abulte commented Jul 23, 2025

Copy link
Copy Markdown
Contributor

This won't remove the branch from the input UI on GitHub, right? Feels weird to ignore a native parameter that works as designed... Like I said previously, this feels a bit over optimized and #802 will simplify the workflow anyway.

Yes it seems the branch selector in the input UI on GitHub is native and can't be removed. From what I understood from your previous message I thought you were suggesting to keep the PR selector, sorry from misunderstanding. So we want no PR input in the GUI, only the native branch one (I agree), but the PR number deduced from the branch? I would think we can deduce a branch from a PR number, but I wouldn't be sure about the opposite, as there can be several PRs from the same branch?

To be clear, I think the current state is fine. It's a bit painful to input both branch and PR number but I can live with it, and anyway I'll use #802 when it's merged. Makes more sense anyway, since the comment "knows" on which PR we're working.

@bolinocroustibat bolinocroustibat changed the title ci: fix manual review app deployment workflow ci: fix manual review app deployment instructions Jul 23, 2025

@abulte abulte left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙏

@bolinocroustibat bolinocroustibat merged commit 8b18201 into main Jul 23, 2025
15 checks passed
@bolinocroustibat bolinocroustibat deleted the fix-review-app-ci branch July 23, 2025 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Error on review app deployment when not using my PR's branch

2 participants