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: Bump up crossplane-runtime version to v1.18.0 #113

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

cychiang
Copy link
Contributor

@cychiang cychiang commented Dec 28, 2024

Description of your changes

  • Bump up crossplane-runtime to v1.18.0
  • Fix linting issue by bumping up build submodules to latest one
  • Add support for logs that introduced in v1.17.0
  • Add support for metrics that introduced in v1.16.0
  • Update Delete function that required changes in v1.17.0
  • Bump up go version to 1.22

This PR is related to crossplane/crossplane#5924, and solve part of it.

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

* Fix linting issue by bumping up build submodules to latest one
* Add support for logs that introduced in v1.17.0
* Add support for metrics that introduced in v1.16.0
* Update Delete function that required changes in v1.17.0
* Bump up go version to 1.22

Signed-off-by: Chuan-Yen Chiang <[email protected]>
@cychiang cychiang force-pushed the chore-bump-up-crossplane-runtime-to-1-18-0 branch from 065d84b to b53779e Compare December 30, 2024 08:41
Copy link
Member

@jbw976 jbw976 left a comment

Choose a reason for hiding this comment

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

very cool to see the classic provider template get some attention! thank you for taking the time to do that 🙇‍♂️

can you provide a few more details about how you tested that these changes work OK? is there a test/dummy provider build that you used the updated template for?

also, what was your general approach to get all these updates? did you follow the model of a particular up to date provider, look at the release notes, etc.?

@@ -126,6 +151,22 @@ func main() {
log.Info("Alpha feature enabled", "flag", features.EnableAlphaManagementPolicies)
}

if *enableChangeLogs {
Copy link
Member

Choose a reason for hiding this comment

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

we may want to hold off an change log specific updates because they won't end to end until we have a real sidecar container build/published. I can't remember exactly what will happen if there's no server injected and present, but I can imagine the managed reconciler will encounter errors, or maybe even hang for periods of time.

See my note on crossplane/crossplane#5924 (comment) for more context on the need to get this sidecar gRPC server to the finish line! 😇

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then I think I can remove this part for now, and bring it back when its ready.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like, what if in the near future, if someone enables an change log but failed to have a validate DeploymentRuntimeConfig? It seems like a potential risk, and might need to handle this from the crossplane-runtime? But I can try it out a bit and see what would happened... 🤔

package version

// Version will be overridden with the current version at build time using the -X linker flag
var Version = "0.0.0"
Copy link
Member

@jbw976 jbw976 Jan 25, 2025

Choose a reason for hiding this comment

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

hmm, interesting to see that the Makefile already seemed to have the GO_LDFLAGS referencing this variable, weird to see that this accompanying file wasn't present too. Maybe the file doesn't actually need to exist, but it's helpful to have in the source tree with this comment explaining how it works?

// SPDX-FileCopyrightText: 2024 The Crossplane Authors <https://crossplane.io>
Copy link
Member

Choose a reason for hiding this comment

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

did you already see where/how these headers may be used for build tool automation? i haven't seen that myself yet 🤔 i.e., is there more that would be needed besides this header update?

Copy link
Contributor Author

@cychiang cychiang Jan 25, 2025

Choose a reason for hiding this comment

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

I think I update this part according to the feedback from a PR in provider-upjet-template. It seems related to code generator but not sure about it. 😶‍🌫️ Maybe we don't need this?

@cychiang
Copy link
Contributor Author

very cool to see the classic provider template get some attention! thank you for taking the time to do that 🙇‍♂️

can you provide a few more details about how you tested that these changes work OK? is there a test/dummy provider build that you used the updated template for?

also, what was your general approach to get all these updates? did you follow the model of a particular up to date provider, look at the release notes, etc.?

My general approach for these updates are based on recent changes from crossplane-runtime. Also will try to use the same dependencies as same as crossplane-runtime.

After I finish the change, I will try to build and run in a local environment to test the template-provider is working or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants