-
Notifications
You must be signed in to change notification settings - Fork 211
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
mockgcp: add support for TagsTagBinding #1918
base: master
Are you sure you want to change the base?
mockgcp: add support for TagsTagBinding #1918
Conversation
justinsb
commented
May 31, 2024
- mockgcp: add support for TagsTagBinding
- chore: capture golden output for some tag tests
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3027c92
to
4cd1934
Compare
4cd1934
to
fe71af3
Compare
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.
overall this LGTM. Happy to have another look.
err := wait.PollImmediate(1*time.Second, 35*time.Minute, func() (done bool, err error) { | ||
done = true | ||
logger.V(2).Info("Testing to see if resource is ready", "kind", u.GetKind(), "name", u.GetName()) |
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.
here and below for samples.go -- do we want to disassociate the logs? Thinking that it may be irrelevant for whne running a single test but maybe useful for when running multiple?
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.
What do you mean by "disassociate the logs" ?
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.
(If you mean "why did we stop adding the values, it's because we add them to the logger above:
logger := log.FromContext(t.Ctx).WithValues("kind", gvk.Kind, "name", id.Name)
)
# TODO: projectId should work | ||
external: //cloudresourcemanager.googleapis.com/projects/${projectNumber} |
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 may have lost state on this question, but are we going to make project-${uniquedId}
work as a follow on or test that it works as a follow on?
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.
Yes, I think we should make it work when we have a direct actuation implementation!
e7f50a3
to
f5a9777
Compare
Co-authored-by: alex <8968914+acpana@users.noreply.github.com>
f5a9777
to
b2e7fc5
Compare