-
Notifications
You must be signed in to change notification settings - Fork 170
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
Add tenant ID to internal apis for CMSI usage #3655
Conversation
Using #3514 as inspiration |
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! My only comment is the tiniest nit.
/azp run ci, e2e |
Azure Pipelines successfully started running 2 pipeline(s). |
E2E failed with what appear to be flakes. I'm going to investigate a bit and see if I can card up some tickets to address them, but in the meantime, I'll trigger E2E again. |
/azp run e2e |
Azure Pipelines successfully started running 1 pipeline(s). |
At least one of the flakes seems to be related to some PIP configs that got into a Failed provisioning state somehow. I also saw HTTP connections that were dropped between the E2E agent and the cluster API. I couldn't find anything in ARM logs to explain what I saw, and the Azure resources from the last E2E run are already gone, so I think we'll have to let this one go. |
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 looking for some clarity on the TODOs
// TODO - refactor this function to reduce the number of parameters | ||
func (f *frontend) _putOrPatchOpenShiftCluster(ctx context.Context, log *logrus.Entry, body []byte, correlationData *api.CorrelationData, systemData *api.SystemData, path, originalPath, method, referer string, header *http.Header, converter api.OpenShiftClusterConverter, staticValidator api.OpenShiftClusterStaticValidator, subId, resourceProviderNamespace string, apiVersion string, identityURL string, identityTenantID string) ([]byte, error) { |
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 feel we often leave these types of comments with the best intentions but then never loop back on them.
From just jumping into this I would think we need a little struct here but maybe I'm missing something? I get not adding it here to reduce the PR complexity but could we have a branch off this with the refactor so we know it's all going to go together?
Happy to pair on this too if that helps :)
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.
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 had a little play with it and I think I see where you're coming from. Lots of cans and lots of worms. I opened a small refactror PR here that hopefully helps the next person who comes along and I'm okay to leave it there for now.
Also AFAIK we all own all the code, there are no specific teams that own any part of the codebase but do correct me if I'm wrong.
Which issue this PR addresses:
Part of issue ARO 8606
What this PR does / why we need it:
Test plan for issue:
Unit test
Is there any documentation that needs to be updated for this PR?
No
How do you know this will function as expected in production?