-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Promoting namespace to the GA provider #12644
base: main
Are you sure you want to change the base?
Conversation
Add example file where provider is omitted.
Update `Namespace.yaml` to include the `uid` property and remove instances of `min_version`.
Hello! I am a robot. Tests will require approval from a repository maintainer to run. @shuyama1, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look. You can help make sure that review is quick by doing a self-review and by running impacted tests locally. |
@shuyama1 This PR has been waiting for review for 3 weekdays. Please take a look! Use the label |
mmv1/templates/terraform/examples/service_directory_namespace_ga.tf.tmpl
Outdated
Show resolved
Hide resolved
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.
You'll also need to add a GA endpoint in https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/products/servicedirectory/product.yaml
@erikmej-code, this PR is waiting for action from you. If no action is taken, this PR will be closed in 28 days. Please address any comments or change requests, or re-request review from a core reviewer if no action is required. This notification can be disabled with the |
…ga.tf.tmpl Removing file. Not needed.
Updated product.yaml file. |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 13 Click here to see the affected service packages
🔴 Tests were added that are GA-only additions and require manual runs:
Action takenFound 4 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
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.
You'll need to remove the provider = google-beta
in the example file https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/templates/terraform/examples/service_directory_namespace_basic.tf.tmpl#L2, as the example will now be used for GA as well
output: true | ||
- name: 'labels' | ||
type: KeyValueLabels | ||
description: | | ||
Resource labels associated with this Namespace. No more than 64 user | ||
labels can be associated with a given resource. Label keys and values can | ||
be no longer than 63 characters. | ||
min_version: 'beta' | ||
- name: 'uid' |
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.
Any particular reason this is only added to GA? Is this field not supported by the beta API? We require beta provider to be a strict superset of GA, which means whatever in GA should be in beta as well. See 3.2 in https://googlecloudplatform.github.io/magic-modules/code-review/review-pr/#review
You'll likely need to remove this field from the GA implementation as well.
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.
Will fix.
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 13 Click here to see the affected service packages
🔴 Tests were added that are GA-only additions and require manual runs:
View the build log |
HArdcode v1 service directory base path.
@shuyama1 these comments should now be resolved. Please review at your earliest convenience. Thanks! |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 60 Click here to see the affected service packages
🔴 Tests were added that are GA-only additions and require manual runs:
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
Tests analyticsTotal tests: 60 Click here to see the affected service packages
🔴 Tests were added that are GA-only additions and require manual runs:
View the build log |
@@ -4,7 +4,7 @@ func expand{{$.GetPrefix}}{{$.TitlelizeProperty}}(v interface{}, d tpgresource.T | |||
} else if strings.HasPrefix(v.(string), "https://") { | |||
return v, nil | |||
} | |||
url, err := tpgresource.ReplaceVars(d, config, "{{"{{"}}ServiceDirectoryBasePath{{"}}"}}" + v.(string)) |
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.
We generally aim to keep each PR focused on a single change to make issue detection and revert easier. Since this change is separate from the GA promotion, I’ve moved it to a new PR (#12934) and added tests to verify the fix. We can remove this change from the current PR and proceed with the other PR for the basepath override. Please let me know if it works for you. Thanks!
@erikmej-code, this PR is waiting for action from you. If no action is taken, this PR will be closed in 28 days. Please address any comments or change requests, or re-request review from a core reviewer if no action is required. This notification can be disabled with the |
In an effort to promote the namespace to the GA provider we do the following:
min_version
fromNamespace.yaml
and include theuid
field withinproperties
.service_directory_namespace_ga.tf.tmpl
, with the provider omitted.