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

Introducing new go module for API, remove g/g dependency #988

Open
wants to merge 37 commits into
base: master
Choose a base branch
from

Conversation

unmarshall
Copy link
Contributor

@unmarshall unmarshall commented Jan 29, 2025

How to categorize this PR?
/area control-plane
/area dev-productivity
/area open-source
/kind technical-debt
/kind enhancement
/kind api-change

What this PR does / why we need it:

PR introduces the following changes:

  • Carves out 2 new go-modules for API + CRDs and generated typed client.
  • Provides a way to programmatically get to CRDs which can now be used by g/g directly instead of re-generating CRDs and maintaining a copy of it, therefore avoid duplicate CRD maintenance.
  • Now uses the recommended way to generate deepcopy and defaulting functions via https://github.com/kubernetes/code-generator
  • Moves the code generation scripts to the API module
  • ClientSet is now generated for Etcd and EtcdCopyBackupTask CRs which provide any consumers typed clients to interact with these resources.
  • Removes dependency on gardener/gardener packages completely. As part of this line item following was done:
    • Copied ImageVector code (under internal/utils/imagevector as a stop-gap measure till g/g deliberates on moving this functionality into its own go-module).
    • Ported k8s utility functions that were defined in g/g repo to now be part of etcd-druid.
    • Ported constants that were defined in g/g repo to etcd-druid.
  • API enhancement: Introduces PodLabels as part of EtcdCopyBackupsTaskSpec - currently networking specific labels were hard coded which were very specific to gardener usage. Now the consumers can pass in any additional labels that needs to be applied to the Pods that are started as part of a Job that gets created.
  • For some of the packages that were touched as part of this PR, ginkgo tests were replaced with golang native tests Packages/Files for which this has been done:
    • Package: internal/controller/secret
    • New Package: internal/utils/kubernetes
    • Added missing test file for internal/utils/miscellaneous.go
  • Fixed skaffold.yaml - prior to this PR, consumers were forced to do a gcloud auth login even though we were starting the kind cluster with local docker registry. This PR adds a small fix to skaffold.yaml which removes the need to run gcloud auth login.
  • Structure of charts have been simplified. Following are the changes done:
    • Directory structure simplified.
    • Switched to using intrinsic support for CRDs. CRDs are not duplicated and checked-in to git repo. Instead they will be copied on demand via running hack/prepare-helm-charts.sh script.
    • Removed generated PKI artifacts. These will now be generated via hack/prepare-heml-charts.sh and hack/openssl-util.sh scripts.
    • Now leveraging .Files.Get and b64enc functions to bring in the PKI artifacts.
  • Removed etcd-druid/config directory as this was not maintained. Moved out config/samples to etcd-druid/samples
  • Added git_tags to .ci/pipeline_definition allowing proper tagging for go modules when release is done.

Note

You will notice that the API directory structure introduces api/core/v1alpha1. This is deliberately done. This allows introduction of operator config in subsequent PR (under api/config/v1alpha1 directory) which will completely replace the existing CLI args. The code generation scripts will be used to generate defaulting functions for the operator config.

Note

go.work and go.work.sum are added to the repo. This will make developing across different go modules easier especially when there are dependencies amongst them. In addition, if you remove these files then make sast will stop working (see securego/gosec#1100 (comment) for mode details).

Which issue(s) this PR fixes:
Fixes #972 #968 #969 and partially resolves 940

Special notes for your reviewer:

Release note:

* API import path has changed, it is now github.com/gardener/etcd-druid/api/core/v1alpha1
* Annotation key to reconcile an Etcd cluster has been changed to `druid.gardener.cloud/operation` from `gardener.cloud/operation`
It is recommended that consumers of etcd-druid should depend upon the API go module instead of the parent go-module. In go.mod your dependency should be on `github.com/gardener/etcd-druid/api`.
It is now possible to get the CRDs as string via the go API.
With this PR, dependency on utilities from https://github.com/gardener/gardener has been removed.
EtcdCopyBackupTask introduces a new field spec.PodLabels which should be used to setup additional labels on the pod(s) that are started for etcd-copy-backup operation.
To run use skaffold via 'make deploy*' targets it is no longer required to run 'gcloud auth login'
Samples for etcd-druid are now present at `etcd-druid/samples` directory
PKI resources required for helm charts are now generated on the fly when running 'make deploy*' targets.
CRDs are no longer checked-in at 2 places. There is a single source of truth for all CRDs and that is 'etcd-druid/api/core/crds'. Prior to using 'make deploy*' targets this will be copied to 'etcd-druid/charts/crds' directory.
Removed the unmaintained support for deployment of etcd-druid via kustomize
Generated etcd-druid client is now its own go module. This go module provides access to a typed client for all custom resources introduced in the API go module. This will ensure that consumers will always have a lean dependency weight when using the client.
Added convenience make targets
* 'clean-chart-resources' which will clean up all copied and generated resources prior to using helm charts
* 'prepare-helm-charts' which will prepare the helm charts by copying the CRD yamls and generating the PKI resources for the webhook.
Added a document to describe how to use the etcd-druid helm charts

@unmarshall unmarshall requested a review from a team as a code owner January 29, 2025 10:49
@gardener-robot gardener-robot added needs/review Needs review area/control-plane Control plane related area/dev-productivity Developer productivity related (how to improve development) area/open-source Open Source (community, enablement, contributions, conferences, CNCF, etc.) related kind/api-change API change with impact on API users kind/enhancement Enhancement, improvement, extension kind/technical-debt Something that is only solved on the surface, but requires more (re)work to be done properly needs/rebase Needs git rebase labels Jan 29, 2025
@gardener-robot
Copy link

@unmarshall You need rebase this pull request with latest master branch. Please check.

@gardener-robot gardener-robot added size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py) needs/second-opinion Needs second review by someone else labels Jan 29, 2025
@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jan 29, 2025
@unmarshall unmarshall marked this pull request as draft January 29, 2025 11:06
@shreyas-s-rao shreyas-s-rao added this to the v0.29.0 milestone Jan 29, 2025
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jan 29, 2025
@gardener-robot-ci-2 gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jan 29, 2025
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jan 29, 2025
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jan 30, 2025
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jan 30, 2025
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jan 30, 2025
@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jan 30, 2025
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jan 30, 2025
…and fixed skaffold, no need to have gcloud auth login
… for helm, corrected helm charts, created client as go module
…le target to clean generated resources for chart
… pki resources and fixed skaffold debug profile config
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Feb 8, 2025
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Feb 8, 2025
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Feb 8, 2025
@gardener-robot-ci-2 gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Feb 8, 2025
@unmarshall
Copy link
Contributor Author

/retest

Copy link

gardener-prow bot commented Feb 9, 2025

@unmarshall: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-etcd-druid-integration e682cd1 link true /test pull-etcd-druid-integration
pull-etcd-druid-e2e-kind e682cd1 link true /test pull-etcd-druid-e2e-kind

Full PR test history. Your PR dashboard. Command help for this repository.
Please help us cut down on flakes by linking this test failure to an open flake report or filing a new flake report if you can't find an existing one. Also see our testing guideline for how to avoid and hunt flakes.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/control-plane Control plane related area/dev-productivity Developer productivity related (how to improve development) area/open-source Open Source (community, enablement, contributions, conferences, CNCF, etc.) related kind/api-change API change with impact on API users kind/enhancement Enhancement, improvement, extension kind/technical-debt Something that is only solved on the surface, but requires more (re)work to be done properly needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/rebase Needs git rebase needs/review Needs review needs/second-opinion Needs second review by someone else size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reduce dependencies on g/g
7 participants