-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
added functionality in mmv1 to support configurable sweepers #10650
Conversation
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR hasn't generated any diffs, but I'll let you know if a future commit does. |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR hasn't generated any diffs, but I'll let you know if a future commit does. |
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: Click here to see the affected service packages
|
Co-authored-by: Cameron Thornton <camthornton@google.com>
mmv1/provider/terraform/sweeper.rb
Outdated
class Sweeper < Google::YamlValidator | ||
# The field checked by sweeper to determine | ||
# eligibility for deletion for generated resources | ||
attr_reader :tf_test_field |
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 was a little confused at what this was at first... can you think of a better name for this? I can't think of anything perfect, but maybe sweepable_name_field
or something?
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.
Yea, I was also struggled to find a better name for it. I'll update it with a more clear name, sweepable_name_field
seems a good one
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.
change it to sweepable_identifier_field
. let me know if that makes sense
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.
a lot clearer thanks!
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.
LGTM, just have an opinion on attribute name
As far as Go rewrite, we haven't gotten to sweepers yet, so this should be good to add right now. |
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: Click here to see the affected service packages
|
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: Click here to see the affected service packages
|
…loudPlatform#10650) Co-authored-by: Cameron Thornton <camthornton@google.com>
…loudPlatform#10650) Co-authored-by: Cameron Thornton <camthornton@google.com>
…loudPlatform#10650) Co-authored-by: Cameron Thornton <camthornton@google.com>
…loudPlatform#10650) Co-authored-by: Cameron Thornton <camthornton@google.com>
fixes hashicorp/terraform-provider-google#16859
Release Note Template for Downstream PRs (will be copied)