-
Notifications
You must be signed in to change notification settings - Fork 169
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
Use single ARO API/client version in pkg/util/cluster #3583
Use single ARO API/client version in pkg/util/cluster #3583
Conversation
/azp run ci,e2e |
Azure Pipelines successfully started running 2 pipeline(s). |
a9998d7
to
797f933
Compare
/azp run ci,e2e |
Azure Pipelines successfully started running 2 pipeline(s). |
797f933
to
c4e02fa
Compare
/azp run ci,e2e |
Azure Pipelines successfully started running 2 pipeline(s). |
@@ -169,9 +160,9 @@ func (c *Cluster) DeleteApp(ctx context.Context) error { | |||
} | |||
|
|||
func (c *Cluster) Create(ctx context.Context, vnetResourceGroup, clusterName string, osClusterVersion string) error { | |||
clusterGet, err := c.openshiftclustersv20230904.Get(ctx, vnetResourceGroup, clusterName) | |||
clusterGet, err := c.openshiftclusters.Get(ctx, vnetResourceGroup, clusterName) |
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'm sure you've thought of this, but for the time being, does it make sense to just wrap this into an if isDevelopment
or similar block, and have prod e2es use the old client? It would make for cleanup later, but not sure how else we'd merge this before the v20240812preview API is available everywhere.
This has the downside of e2e testing the new API in dev but the old API in prod, which might be useful for smoke testing the new API, but would create a slight difference between the e2e runs depending on environment.
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.
Team Loki talked about this in our sync PR review session, this proposed solution is somewhere on the spectrum between the current implementation in this PR, and a long-term idea we have to make the API version used here configurable (e.g. allow us to run E2E against multiple API versions), something that may become more desirable for us as we progress further along the CI/CD initiative.
I would like to implement this proposed solution in the near-term, but I think it would require some thought into re-usability for that potential later step, and thus I think it makes the most sense in a follow-up PR.
For now, I'm thinking the steps are:
- This PR: downgrade the single API version used to our latest routeable API (20230904)
- Follow-up PR: implement some sort of determination between the latest routeable and in-progress APIs, e.g. through an abstracted client implementation that accepts requests and provides responses with the internal clusterdoc representation, and converts to the specific external representation before actually amking the API call (which is how pkg/util/cluster operates today anyways)
- Future effort, depending on need: allow pkg/util/cluster to be instantiated with a specific API version, implement a configurable parameter in both PR and prod E2E to specify the version to use (default being the behavior above)
/azp run ci,e2e |
Azure Pipelines successfully started running 2 pipeline(s). |
/azp run e2e |
Azure Pipelines successfully started running 1 pipeline(s). |
version for everything I don't see why this change is needed, and #3583 was just recently merged. I'm thinking that maybe Caden added these changes by mistake while mid-merge.
version for everything I don't see why this change is needed, and #3583 was just recently merged. I'm thinking that maybe Caden added these changes by mistake while mid-merge.
version for everything I don't see why this change is needed, and #3583 was just recently merged. I'm thinking that maybe Caden added these changes by mistake while mid-merge.
Which issue this PR addresses:
Fixes no Jira
What this PR does / why we need it:
Updates pkg/util/cluster/cluster.go to use a single ARO API version and client, replacing the 4 different API versions previously in use.
This version is currently set to the v20230904 API - the latest public-facing API as of now. This allows the functionality in pkg/util/cluster to continue to be used against production RP instances, e.g. in production E2E contexts.
Test plan for issue:
Is there any documentation that needs to be updated for this PR?
No.
How do you know this will function as expected in production?
N/A (this functionality is only used during local development and when running E2E)