-
Notifications
You must be signed in to change notification settings - Fork 52
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
Adding in support for a "No Project" setting for gcp:project
#2653
base: master
Are you sure you want to change the base?
Conversation
2c05a7f
to
4e1d52b
Compare
Does the PR have any schema changes?Looking good! No breaking changes found. Maintainer note: consult the runbook for dealing with any breaking changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this pull request, Richard! 💟
In addition to my nits 😅 would you please add to the PR description:
- briefly describe the change (what)
- what this change is addressing (why)
- how it is addressing the issue (how)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like DisableGlobalProjectWarning
much more then project: "No Project"
.
Can you please link the issue you are addressing and explain your fix in the PR body. If users see that the underlying issue is fixed, they should understand at minimum how to apply the fix for themselves.
Absolutely, will do. |
0416cd6
to
642c6b8
Compare
gcp:project
642c6b8
to
5f715d4
Compare
Co-authored-by: Ian Wahbe <[email protected]>
1313c75
to
152540b
Compare
152540b
to
647c9ff
Compare
@@ -169,6 +169,10 @@ const ( | |||
gcpWorkstations = "Workstations" // Workstations | |||
) | |||
|
|||
// When `gcp:project` is set to noProjectMarker, we don't warn that a global project is missing | |||
// and we don't try to validate the project. | |||
const noProjectMarker = "DisableGlobalProjectWarning" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a bit late to the party here but should we perhaps add a new config to disable the warning instead? That seems like a nicer UX for users than a magic string in place of the project config and a bit easier to discover.
e.g.
So instead of:
gcp.Provider(project="DisableGlobalProjectWarning")
users would use
gcp.Provider(disable_global_project_warning=true)
You can define that here:
pulumi-gcp/provider/resources.go
Line 534 in a30bcc5
"skipRegionValidation": { |
We have an existing config we've added to disable another similar check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The benefit of this is that it would automatically get added to our docs etc. It would also be discoverable as a property on the ProviderArgs.
It might also work better with other things in the provider as I do not know all the places where the project is used - setting it to an invalid string might interact in odd ways with other things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rshade LMK what you think, also happy to help with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iwahbe @guineveresaenger thoughts? I am good any way we do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is definitely better.
gcp.Provider(disable_global_project_warning=true)
When I saw DisableGlobalProjectWarning
I initially assumed that ☝️ is what was meant (which I then promptly forgot to right down when I saw the code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
excellent thank you @VenelinMartinov @iwahbe I will get that put in on Monday.
fixes #1168