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.

Sign up for GitHub

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

Jump to bottom

Add option to set / skip CloudBuildNumber #1190

Merged
merged 5 commits into from
Apr 1, 2025
Merged

Add option to set / skip CloudBuildNumber #1190

merged 5 commits into from
Apr 1, 2025

Conversation

MattKotsenas
Copy link
Member

Fixes #1189

This PR adds the ability to set / skip the CloudBuildNumber in both the Cake task and the CLI.

Common

  • Adds a new bool cloudBuildNumber to CloudCommand::SetBuildVariables
     

Cake.GitVersioning

  • Adds a new CloudBuildNumber to GetVersioningCloudSettings with the default value of true
  • Makes the default values of AllVariables and CommonVariables explicit (but no change)

nbgv CLI

  • Adds a new command line switch --skip-cloud-build-number / -b which if specified sets cloudBuildNumber to false.

Tests

  • There weren't any automated tests to cover this scenario, so added some basic tests

@MattKotsenas
Copy link
Member Author

MattKotsenas commented Mar 23, 2025 •
edited
Loading

I felt bad not writing tests for this, but I'm not happy with the tests that I wrote:

  • They're tied to individual cloud providers, which feels wrong
  • They test by side effect, assuming that if all + common + cloudBuildNumber are all off, the output will always be empty

I'm happy to refactor CloudCommand to take an ICloudBuild for testing, but the only ways I can think of are to either make a public constructor or make an internal one and add InternalsVisibleTo. Let me know if you have any preference.

AArnott
AArnott requested changes Mar 24, 2025
View reviewed changes
Copy link
Collaborator

@AArnott AArnott left a comment

Choose a reason for hiding this comment

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

Thanks for this. I left a comment on a suggested test change, but otherwise I'm happy to take this.

I also removed the = false; property initializers you added since I don't care for explicitly setting defaults.

@MattKotsenas MattKotsenas requested a review from AArnott March 28, 2025 18:11
@AArnott AArnott added this to the v3.8 milestone Apr 1, 2025
@AArnott AArnott merged commit bba85d9 into dotnet:main Apr 1, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AArnott AArnott AArnott approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
v3.8
Development

Successfully merging this pull request may close these issues.

Option to not emit vso[build.updatebuildnumber] to prevent pipeline warnings
2 participants
@MattKotsenas @AArnott