Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disallow deploying branches out-of-date with main #235

Closed
ghost opened this issue Jan 19, 2024 · 5 comments
Closed

Disallow deploying branches out-of-date with main #235

ghost opened this issue Jan 19, 2024 · 5 comments
Assignees
Labels
enhancement New feature or request

Comments

@ghost
Copy link

ghost commented Jan 19, 2024

My team relies on the branch-deploy functionality to prevent deploying branches that are out of date (i.e. update_branch = "warn"). We noticed recently that there's a nuance to this though: it only prevents deploying PRs that are out of date with their base branch.

My team uses a stacked diffs workflow; the main relevant effect of that here is that it's not uncommon for branches to be stacked on top of each other - i.e. featureA's base branch is main, but featureB's base branch is featureA. As-is, branch deploy would allow us to deploy featureB even if the stack including featureA and featureB is out of date when comparing against main.

For the time being we have a workaround by just adding our own condition to our action that checks the result of

git rev-list --left-right --count origin/main..origin/featureB | awk '{print $2}'

but in an ideal world, it would be great if this could be implemented by branch-deploy internally.

Something along the lines of an input like outdated_mode with options pr_base and default_branch (defaulting to pr_base to prevent this being a breaking change).

@GrantBirki GrantBirki added the enhancement New feature or request label Jan 19, 2024
@GrantBirki
Copy link
Member

@jessew-albert thank you for this suggestion! I am always pleased to learn about new ways folks are using this Action. I already have a few ideas in mind for how to implement a new feature that would enable your team to use this workflow better. The first idea that comes to mind would be implementing a new input option into this Action that allows you to configure what branch this Action is looking at to determine its "base".

I'm currently on holiday traveling around the UK and Scandinavia, but I have a few long train rides coming up so perhaps I can work on opening a PR while riding a railway 🚂 .

Thanks! ❤️

@GrantBirki GrantBirki self-assigned this Jan 19, 2024
This was referenced Jan 28, 2024
@GrantBirki
Copy link
Member

@jessew-albert Clarifying question for you:

Are you looking to have this Action internally compare a different branch than the pull request base? Currently, this Action only cares about the branch that your pull request is merging into. So if your stack of PRs is out-of-date with main but not out-of-date with each other, then you should be okay.

To think about it visually, this Action will warn/force update the branch if the "update branch" (gray button), exists at the bottom of your pull request.

@ghost
Copy link
Author

ghost commented Jan 31, 2024

@GrantBirki not sure I'm following your question - the current behavior is what you described, i.e. if the stack of PRs is out of date with main but not out of date with each other, then branch deploy will allow it to be deployed... but I'm looking for a way to get branch deploy to consider a branch out of date if it is out of date with main.

In context of how my team uses the action, our main branch maps 1:1 to our dev environment (i.e. when things get merged to main, they auto-deploy to dev). Then branch-deploy can be used to briefly, sort of "temporarily" deploy changes on feature branches to the dev environment while testing them but before merging to main. We don't want to have a branch get deployed to dev that is behind the main branch though. That is, there should never be code deployed to dev that is missing commits that are on main. That should hold true whether the base of the PR is main or another feature branch (as is common when using stacked diff tooling such as Graphite).

Here's a diagram of when I'd expect branch-deploy to warn about an outdated branch:

gitGraph
   commit id: "A"
   commit id: "B"
   branch featureA
   checkout featureA
   commit id: "featA1"
   commit id: "featA2"
   branch featureB
   checkout featureB
   commit id: "featB1"
   commit id: "featB2"
   checkout main
   commit id: "C"
   commit id: "D"
Loading

In this scenario, the gray "update branch" button would not show in featureB's PR, because featureB is not out of date with its base (featureA).

However, if I tried to deploy featureB to dev using branch-deploy, that shouldn't be allowed, because featureB is missing the 2 most recent commits to main - therefore, if I deployed featureB to dev, I would effectively be rolling dev back 2 commits unexpectedly. In order for deploying featureB to be allowed, we'd need to either merge main into featureB, i.e.

gitGraph
   commit id: "A"
   commit id: "B"
   branch featureA
   checkout featureA
   commit id: "featA1"
   commit id: "featA2"
   branch featureB
   checkout featureB
   commit id: "featB1"
   commit id: "featB2"
   checkout main
   commit id: "C"
   commit id: "D"
   checkout featureB
   merge main
Loading

or rebase the stack with featureA and featureB on top main, i.e.

gitGraph
   commit id: "A"
   commit id: "B"
   commit id: "C"
   commit id: "D"
   branch featureA
   checkout featureA
   commit id: "featA1"
   commit id: "featA2"
   branch featureB
   checkout featureB
   commit id: "featB1"
   commit id: "featB2"
Loading

In my opinion this feels like it should be the default behavior of branch deploy, although it's possible I'm wrong and that there's workflows out there that wouldn't want this behavior. I think if this behavior could be configured via an input that should help accommodate any workflow... please let me know if I'm making sense here!

Also thanks very much for your effort, I really love this action!

@GrantBirki
Copy link
Member

@jessew-albert thank you so much for your very detailed explanation! It was the diagrams that really helped me wrap my head around a solution here so I am very grateful for them (they even made it into the docs!).

I have published a pre-release version of this Action and have done a lot of acceptance testing with the new changes. The release is v9.0.0.

Please go ahead and give the release a go and see if the new default outdated_mode: "strict" works for you and solves this issue. If all looks okay, let me know and I'll close out this issue and publish the release officially.

Thank you for all the help here! 🎉

Happy deployments 🚀

@ghost
Copy link
Author

ghost commented Feb 2, 2024

Thanks so much for the work @GrantBirki! I just tried out the 9.0.0 release and it's all looking good to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant