Skip to content

Wait as default#929

Merged
dylan-smith merged 26 commits intomainfrom
dylan-smith/wait-as-default
Apr 10, 2023
Merged

Wait as default#929
dylan-smith merged 26 commits intomainfrom
dylan-smith/wait-as-default

Conversation

@dylan-smith
Copy link
Contributor

@dylan-smith dylan-smith commented Apr 1, 2023

  • Did you write/update appropriate tests
  • Release notes updated (if appropriate)
  • Appropriate logging output
  • Issue linked
  • Docs updated (or issue created) - DOCS: Update documentation to remove --wait flag #946
  • New package licenses are added to ThirdPartyNotices.txt (if applicable)

Implements #705

Changing the default behavior of migrate-repo and migrate-org to be the same as the --wait flag.

The --wait flag is now unnecessary, but for compat reasons I'm leaving it in. I made it a hidden option, and if you pass it you'll get this warning: --wait flag is obsolete and will be removed in a future version. The default behavior is now to wait.

Added a new option --queue-only which will result in the same behavior as the previous default behavior.

Also updating generate-script commands to include the --queue-only flag for parallel scripts (and remove --wait from the sequential scripts).

For customers that have pre-existing parallel scripts (without the --wait flag) this will result in a change in behavior (scripts that used to be parallel will now run sequentially), but it shouldn't break their scripts (the integration tests confirm this). Anybody that relies on the default behavior will start getting a warning that says:
[WARNING] The default behavior has changed from only queueing the migration, to waiting for the migration to finish. If you ran this as part of a script to run multiple migrations in parallel, consider using the new --queue-only option to preserve the previous default behavior.. In a future release we'll remove this warning.

Any existing sequential scripts will continue to work just fine, they will just see warnings about using an unnecessary --wait flag now.

Added validation that both --wait and --queue-only can't be passed in together.

@github-actions
Copy link

github-actions bot commented Apr 1, 2023

Unit Test Results

751 tests   751 ✔️  24s ⏱️
    1 suites      0 💤
    1 files        0

Results for commit d2c6614.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@timrogers timrogers left a comment

Choose a reason for hiding this comment

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

There definitely are breaking change concerns with this update, but I still think we should make the jump and fix this behaviour - especially before GA.

At worst, it'll be a minor annoyance if your script which used to run now waits, and you can change it!

What do you think of adding a temporary warning for a version or two to give users a heads-up about the change in behaviour?

@dylan-smith
Copy link
Contributor Author

There definitely are breaking change concerns with this update, but I still think we should make the jump and fix this behaviour - especially before GA.

At worst, it'll be a minor annoyance if your script which used to run now waits, and you can change it!

What do you think of adding a temporary warning for a version or two to give users a heads-up about the change in behaviour?

Just to make sure I understand your suggestion, you are suggesting we first roll out a change that adds the --queue-only flag, but leaves the default behavior the same as today, and spits out a warning if you don't pass either --wait or --queue-only saying something like the default behavior will change in the future, you should pass --queue-only flag instead of relying on default behavior. Then a few weeks down the road we roll out the change to the default behavior. Then a couple months after that we remove the --wait flag entirely.

Because I considered doing it that way, but the fact that we are not actually breaking anybodies scripts, we're just making them run slower made me lean towards just pulling the bandaid off all at once.

But I'll defer to your opinion on this one.

@timrogers
Copy link
Contributor

I was thinking that we change the default immediately, but add a temporary log line as a heads up about the change.

@dylan-smith
Copy link
Contributor Author

I was thinking that we change the default immediately, but add a temporary log line as a heads up about the change.

Oh I see, so maybe a warning is somebody runs it without either --wait or --queue-only to let them know the behavior has changed.

@timrogers
Copy link
Contributor

Exactly. That's what I was thinking.

@dylan-smith dylan-smith marked this pull request as ready for review April 10, 2023 15:44
Output = new FileInfo(OUTPUT),
Verbose = true,
NoSslVerify = true
NoSslVerify = true,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: extra comma

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's on purpose, and generally considered a good practice. Makes it easier to add new lines/properties after it, and the subsequent diff will be cleaner.

@github-actions
Copy link

Code Coverage

Package Line Rate Branch Rate Complexity Health
bbs2gh 82% 79% 641
gei 86% 82% 738
Octoshift 86% 74% 1129
ado2gh 84% 82% 667
Summary 85% (6960 / 8182) 79% (1738 / 2208) 3175

@dylan-smith dylan-smith merged commit 292869c into main Apr 10, 2023
@dylan-smith dylan-smith deleted the dylan-smith/wait-as-default branch April 10, 2023 20:33
timrogers added a commit that referenced this pull request Jul 25, 2023
…ate-repo` and `migrate-org`

Up until April, the default behavior of `migrate-repo` and
`migrate-org` was to exit immediately after queueing the
migration, and not to wait for it to complete. Users could opt
in to the "wait" behaviour using the `--wait` option.

In April, we [flipped][1] these defaults around, making waiting
the default and adding the `--queue-only` option for the old
behavior.

This meant that the `--wait` option could go away - but we wanted
to do this in a user-friendly, backwards-compatible way.

At that time, we deprecated the `--wait` option, hiding it from
the in-CLI documentation and printing warning when it's used.
These warnings have been in place for 3 months, so now is a
reasonable time to rip off the band aid and remove the argument.

After this change is released, specifying the old `--wait` option
will cause the CLI to error.

Fixes #1082 and
#1059.

[1]: #929
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.

3 participants