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

Rename --only-regions, remove primary regions flag #3514

Merged
merged 3 commits into from
May 2, 2024

Conversation

benbjohnson
Copy link
Contributor

@benbjohnson benbjohnson commented May 2, 2024

Change Summary

This pull request removes the previous --region flag on fly deploy as it overrode the primary-region setting in the config, which was surprising to say the least. I was originally going to rename it to --primary-region but I don't see a good use case for overriding the primary region for a single deploy. We can add it back in if it's useful.

I also renamed the --only-regions to --regions but retained the --only-regions alias for backwards compatibility.

Fixes: #3509


Documentation

  • Fresh Produce
  • In superfly/docs, or asked for help from docs team
  • n/a

@benbjohnson benbjohnson requested a review from tvdfly May 2, 2024 15:28
Copy link
Member

@btoews btoews left a comment

Choose a reason for hiding this comment

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

Rad! Thanks for taking care of this.

internal/command/deploy/deploy.go Outdated Show resolved Hide resolved
Copy link
Contributor

@tvdfly tvdfly left a comment

Choose a reason for hiding this comment

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

💖

@benbjohnson benbjohnson merged commit 363a040 into master May 2, 2024
39 checks passed
@benbjohnson benbjohnson deleted the regions-filter branch May 2, 2024 21:26
@ghost
Copy link

ghost commented May 2, 2024

Hi @benbjohnson , might be worth updating your example usages so that people aren't confused when viewing example usages after running the old --region. This just broke our deploy and the example usage still mentions --region

--regions strings Deploy to machines only in these regions. Multiple regions can be specified with comma separated values or by providing the flag multiple times. --region iad,sea --regions syd will deploy to all three iad, sea, and syd regions. Applied before --exclude-regions. V2 machines platform only.

@jroes
Copy link

jroes commented May 3, 2024

Just FYI - this kind of change is breaking, and can bust CI/CD workflows, which I just experienced. Might be nice to alias the old behavior or something. (I'm personally using a fork of https://github.com/superfly/fly-pr-review-apps)

@alexspeller
Copy link

This also broke our CI workflow - and this is the second time this has happened in the past couple of months - please can you consider some kind of formal deprecation process with release notes to users. It's really hard to rely on fly for our production environments at the moment.

@damianopetrungaro
Copy link

Our CI workflow broke as well - on top of that we had to fork this repo to support some other basic flags.

We need to have a way more stable deprecation process on this tools, otherwise we're gonna have to handle more friction on our side (which is the whole reason why we're using fly as provider).

@supriyo-biswas
Copy link

Adding another comment about breakage to our CI deployment process as well.

In fact, in this case --regions behaves almost the same as --region, so I don't see why the deprecation needed to happen at all.

On my side, I guess I'm just gonna have to pin the deployment version.

@benbjohnson
Copy link
Contributor Author

Thanks for the feedback everyone and I apologize for the breakage. I'll do a better job handling this in the future.

in this case --regions behaves almost the same as --region, so I don't see why the deprecation needed to happen at all.

@supriyo-biswas The --region flag on fly deploy changes the primary region that the application runs in whereas --regions specifies a list of regions to deploy to. The change was made because updating the primary region can cause very unexpected changes in some apps when all you meant to do was just deploy to a subset of regions.

@remorses
Copy link

remorses commented May 15, 2024

I opened an issue to discuss this problem more in depth: #3551

This change is very dangerous, people may simply replace --region with --regions but the behaviour is completely different and will cause having machines with older image versions running side by side with the new machines.

Imagine what someone that was previously using --region may do when the flag is no longer working:

  1. User tries to deploy, --region no longer works
  2. Finds the new --regions flag, just replaces it
  3. The user passes a new region in this flag thinking the command will simply deploy machines in that region, like it did previously
  4. The command instead only runs the deployment ignoring regions not passed to --regions, causing the old machines to keep running with the old image version, possibly breaking production

This just happened to me 2 different times breaking prod, I didn't really understand why at first

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.

fly deploy -r changes primary region
8 participants