Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Update kubernetesConfiguration.fluxConfiguration resource version to 2024-11-01 #4638

Merged

Conversation

arnaud-tincelin
Copy link
Contributor

@arnaud-tincelin arnaud-tincelin commented Mar 18, 2025

Add support for kubernetesConfiguration.fluxConfiguration resource to version to 2024-11-01 so that it supports the spec.gitRepository.provider property.

Notes: to generate the code I ran the task cmd but it also updated other resources about ACR. Is this something normal?
I did not write documentation nor tests, did I miss something?

We need this feature ASAP for a customer delivery. Do you have an estimated date for the next ASO release?

  • [] this PR contains documentation
  • this PR contains tests
  • this PR contains YAML Samples

@matthchr
Copy link
Member

Notes: to generate the code I ran the task cmd but it also updated other resources about ACR. Is this something normal?

You've updated the submodule, which brings with it changes upstream that teams have made to their specs. Usually this is just docs but every once in a while there are actual structural changes to existing API versions (almost always because the published spec actually was wrong and didn't work)

I did not write documentation nor tests, did I miss something?

Docs will be automatic. Tests you'll have to add (see my comments)

@theunrepentantgeek
Copy link
Member

Thanks for the PR, much appreciated.

We need this feature ASAP for a customer delivery. Do you have an estimated date for the next ASO release?

We're targeting mid-April for the release of ASO v2.13, which isn't far off.

Also, as soon as your PR merges, we'll automatically publish a new experimental release which you can immediately use for testing.

@theunrepentantgeek
Copy link
Member

The changes under v2/api/machinelearningservices are technically breaking, but they originate upstream.

To simplify things for you, I've pulled out the submodule update into PR #4640 so these changes don't pollute your PR. Once that's merged, you'll need to update your branch.

Copy link
Member

@theunrepentantgeek theunrepentantgeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of minor tweaks, including a missing recording of the sample, but otherwise looks great.

Thanks for your contribution. We're looking forward to merging this.

We need to first merge PR #4640 as a cleanup, then we should be clear to merge this one once CI passes.

@@ -14,7 +14,7 @@ import (
"github.com/Azure/azure-service-operator/v2/internal/util/to"
)

func Test_KubernetesConfiguration_FluxConfiguration_CRUD(t *testing.T) {
func Test_KubernetesConfiguration_FluxConfiguration_20230501_CRUD(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renaming this test is the right thing to do - but you'll need to rerecord the test. This is failing in CI with this error:

=== RUN   Test_KubernetesConfiguration_FluxConfiguration_20230501_CRUD
=== PAUSE Test_KubernetesConfiguration_FluxConfiguration_20230501_CRUD
=== CONT  Test_KubernetesConfiguration_FluxConfiguration_20230501_CRUD
    kube_per_test_context.go:167: creating recorder: required environment variable "AZURE_SUBSCRIPTION_ID" was not supplied
--- FAIL: Test_KubernetesConfiguration_FluxConfiguration_20230501_CRUD (0.00s)

This error appears when there's no recording for the test.

Some background: The test recording is named for the test (so the old test recording is Test_KubernetesConfiguration_FluxConfiguration_CRUD.yaml). But, the name of the test is also used to seed a random number generator that's used to generate unique names within the test, so it can't simply be renamed.

Delete the old file and create a new recording.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I renamed Test_KubernetesConfiguration_FluxConfiguration_CRUD to Test_KubernetesConfiguration_FluxConfiguration_20230501_CRUD and updated the test as it was failing on my side (the flux extension was never installed on the cluster).

I remade several tests (see the commit content)

@theunrepentantgeek
Copy link
Member

Looks like we're at the whack-a-mole stage of getting this merged. 😢
gif

There's another merge conflict, this one for the submodule at v2/specs/azure-rest-api-specs.

Easiest fix is probably to git merge main into your branch.

@arnaud-tincelin arnaud-tincelin force-pushed the feat/upgrade-fluxconfiguration branch from 151778c to 62ad786 Compare March 24, 2025 21:47
@theunrepentantgeek
Copy link
Member

/ok-to-test sha=059f5d3

@theunrepentantgeek theunrepentantgeek added this pull request to the merge queue Mar 26, 2025
Merged via the queue into Azure:main with commit 9c1095d Mar 26, 2025
8 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
new-resouce-version New version of a resource ASO already supports
Projects
Status: Recently Completed
Development

Successfully merging this pull request may close these issues.

3 participants