Skip to content
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

chore: upgrade dcl to v1.62 #1414

Merged

Conversation

acpana
Copy link
Collaborator

@acpana acpana commented Mar 21, 2024

Upgrade to DCL v1.62; release notes

Instructions as per https://github.com/GoogleCloudPlatform/k8s-config-connector/blob/master/README.UpdatingDCL.md where I pinned the v1.62 version.

Comment on lines 235 to 239
{
Kind: "ComputeNetworkAttachment",
DCLType: "NetworkAttachment",
Releasable: false,
},
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This missing entry was causing the CRD generation before.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@justinsb this missing entry was actually causing the CRD generation failure. AFAIU now that failure was to be expected 😅 . I can add this gotcha to our docs for upgrading the lib if that helps.

Also note, this does not upgrade us to latest DCL lib, which last I checked was 1.63 which fails as w a deepcopy issue. I filed #1427 to track that

Signed-off-by: Alex Pana <[email protected]>
@acpana
Copy link
Collaborator Author

acpana commented Mar 22, 2024

Tests ran locally:

eventarc:

# samples
$ go test -v -tags=integration ./config/tests/samples/create -timeout 1600s -test.count=1 -test.run TestAll -run-tests eventarc 2>&1
...
--- PASS: TestAll (1.19s)
    --- PASS: TestAll/eventarctrigger (645.36s)
PASS
...
ok      github.com/GoogleCloudPlatform/k8s-config-connector/config/tests/samples/create 714.968s

# dynamic
$ go test -v -tags=integration ./pkg/controller/dynamic/ -timeout 1600s -test.count=1 -test.run TestCreateNoChangeUpdateDelete -run-tests eventarc 2>&1
...
--- PASS: TestCreateNoChangeUpdateDelete (0.08s)
    --- PASS: TestCreateNoChangeUpdateDelete/eventarc (0.00s)
        --- PASS: TestCreateNoChangeUpdateDelete/eventarc/basic-eventarctrigger (172.16s)
PASS
...
ok      github.com/GoogleCloudPlatform/k8s-config-connector/pkg/controller/dynamic      181.516s

logginglogbucket:

# NOTE: this command excludes the billing-account-log-bucket as it requires special permissions to set up
# samples
$ SOME_ENV_VARS=SET go test -v -tags=integration ./config/tests/samples/create -timeout 1600s -test.count=1 -test.run TestAll -run-tests logginglogbucket 2>&1
...
--- PASS: TestAll (1.18s)
    --- PASS: TestAll/organization-log-bucket (10.08s)
    --- PASS: TestAll/project-log-bucket (10.14s)
    --- PASS: TestAll/project-log-view (11.09s)
    --- PASS: TestAll/folder-log-bucket (34.29s)
PASS
...
ok      github.com/GoogleCloudPlatform/k8s-config-connector/config/tests/samples/create 104.496s

# NOTE: I think this command assumes the extended regex matching in https://github.com/GoogleCloudPlatform/k8s-config-connector/pull/1424
# dynamic
$ SOME_ENV_VARS=SET go test -v -tags=integration ./pkg/controller/dynamic/ -timeout 1600s -test.count=1 -test.run TestCreateNoChangeUpdateDelete -run-tests logginglogbucket 2>&1
...
--- PASS: TestCreateNoChangeUpdateDelete (0.08s)
    --- PASS: TestCreateNoChangeUpdateDelete/logging (0.00s)
        --- PASS: TestCreateNoChangeUpdateDelete/logging/basic-projectlogbucket (65.60s)
        --- PASS: TestCreateNoChangeUpdateDelete/logging/basic-folderlogbucket (84.03s)
PASS
...
ok      github.com/GoogleCloudPlatform/k8s-config-connector/pkg/controller/dynamic      93.865s

@justinsb
Copy link
Collaborator

Nice! I know we had some problems with generation earlier - did those get fixed?

@acpana
Copy link
Collaborator Author

acpana commented Mar 28, 2024

@justinsb I was able to run most dynamic and samples [caveat below] tests for the resources that are changing with 1.62 dcl upgrade. I was missing setting the right vars initially for the logginglogbucket to run locally and jingyi pointed that out to me.


caveat: one samples scenario is not run under samples/resources/logginglogbucket/billing-account-log-bucket due to a permissions issues on the billing account. However, I think we have enough confidence to say that changes to the logginglogbucket are not causing regressions. Wdyt?

@maqiuyujoyce
Copy link
Collaborator

maqiuyujoyce commented Mar 28, 2024

Thank you for verifying using the integration tests, @acpana ! /lgtm

caveat: one samples scenario is not run under samples/resources/logginglogbucket/billing-account-log-bucket due to a permissions issues on the billing account. However, I think we have enough confidence to say that changes to the logginglogbucket are not causing regressions. Wdyt?

Agreed it's likely restricted by your account's permissions (though I still suggest you try to take a look what's missing when you don't have other priorities).

Though I am curious: What's the reason (what features to support/bugs to fix) we upgrade to DCL 1.62? Have you verified the feature is properly supported or bug is fixed in the latest version? Just want to double check because I didn't find any context in the comments / changes in testdata. If there is no specific feature/bugfix to verify, then it's ready to be merged. Otherwise, we probably want to cover the new feature/bugfix with some testdata/sample changes.

@acpana
Copy link
Collaborator Author

acpana commented Mar 28, 2024

What's the reason (what features to support/bugs to fix) we upgrade to DCL 1.62? [...] If there is no specific feature/bugfix to verify, then it's ready to be merged.

AFAICT/ as far as i'm aware, there is no specific reason to upgrade to 1.62 other than trying to get the make upgrade-dcl && ready-pr flow unfixed. This PR upgrades us to 1.62. Then I have #1427 to track 1.63 upgrade failure.

Leaving things as is today (i.e. with make upgrade-dcl && ready-pr broken) can impact contributors who want to add/ modify DCL based resources.

@maqiuyujoyce
Copy link
Collaborator

/approve

@maqiuyujoyce
Copy link
Collaborator

/lgtm
/approve

Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maqiuyujoyce

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-prow google-oss-prow bot merged commit 6dba902 into GoogleCloudPlatform:master Mar 29, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants