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.

Sign up for GitHub

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

Jump to bottom

Update Makefile for simplified local development using Skaffold #599

Merged
merged 5 commits into from
May 31, 2023
Merged

Update Makefile for simplified local development using Skaffold #599

merged 5 commits into from
May 31, 2023

Conversation

seshachalam-yv
Copy link
Contributor

@seshachalam-yv seshachalam-yv commented May 16, 2023 •
edited
Loading

How to categorize this PR?

/area dev-productivity
/kind enhancement

What this PR does / why we need it:
This PR updates the Makefile to use Skaffold for deploying the etcd-druid. It replaces the use of kustomize with Skaffold and it builds the image from the current code base and deploys the pod. This approach simplifies the deployment process and eliminates the need to push the image to the container registry for each local development testing.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Release note:

Developer Action Required: The `make deploy` command has been replaced with `make deploy-via-kustomize`. Please update your deployment workflows accordingly.

Makefile has been updated to use `Skaffold` for deploying `etcd-druid` with the `make deploy` target, simplifying the deployment process and eliminating the need to push the image to the container registry for each local development testing.

@seshachalam-yv seshachalam-yv requested a review from a team as a code owner May 16, 2023 07:09
@gardener-robot gardener-robot added needs/review Needs review area/dev-productivity Developer productivity related (how to improve development) kind/enhancement Enhancement, improvement, extension size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) labels May 16, 2023
@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) 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 May 16, 2023
@seshachalam-yv
Copy link
Contributor Author

Closing this PR as make deploy is using different YAMLs than helm charts by Skaffold.
/close

@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label May 17, 2023
@seshachalam-yv
Copy link
Contributor Author

seshachalam-yv commented May 18, 2023 •
edited
Loading

Reopening the pull request. The skaffold is utilizing the correct YAML files, as evidenced here.
/open

@gardener-robot gardener-robot added status/accepted Issue was accepted as something we need to work on and removed status/closed Issue is closed (either delivered or triaged) labels May 18, 2023
@abdasgupta
Copy link
Contributor

If it's only for local development, can we also find a way to provide desired flags to controller container? We should be able to annotate as well based on Developer's wish

@seshachalam-yv
Copy link
Contributor Author

If it's only for local development, can we also find a way to provide desired flags to controller container? We should be able to annotate as well based on Developer's wish

That needs to be manually changed from the Helm template here if necessary. Subsequently, if we execute make deploy, the corresponding changes will be applied.

abdasgupta
abdasgupta suggested changes May 19, 2023
View reviewed changes
Makefile Show resolved Hide resolved
@gardener-robot gardener-robot added the needs/changes Needs (more) changes label May 19, 2023
@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 May 19, 2023
@seshachalam-yv seshachalam-yv requested a review from abdasgupta May 19, 2023 08:58
@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 May 19, 2023
@abdasgupta
Copy link
Contributor

/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/changes Needs (more) changes needs/review Needs review labels May 19, 2023
@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 May 19, 2023
shreyas-s-rao
shreyas-s-rao requested changes May 23, 2023
View reviewed changes
Copy link
Contributor

@shreyas-s-rao shreyas-s-rao left a comment

Choose a reason for hiding this comment

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

@seshachalam-yv thanks for the PR. Overall LGTM.

  1. Can you please add this line to Makefile target make kind-up to make it easy for developers to set the KUBECONFIG of the new KinD cluster before they run make deploy?
@echo -e "Please run the following command to target the newly created KinD cluster: \n export KUBECONFIG=hack/e2e-test/infrastructure/kind/kubeconfig"

Can you also please add a release note, since this is a breaking change for developers who are actively using make deploy who will now need to move to `make deploy-via-kustomize? Thanks.

Makefile Outdated
Comment on lines 76 to 78
deploy: manifests $(KUSTOMIZE)
kubectl apply -f config/crd/bases
kustomize build config/default | kubectl apply -f -
Copy link
Contributor

Choose a reason for hiding this comment

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

Please provide a new Makefile target make deploy-via-kustomize? Since the repo still supports kustomize-based deployments, and there might be developers who might actually prefer this method, we should not completely remove it, and atleast provide a Makefile target for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, Shreyas, for nice suggestion.
Fixed as part of 231818f.

@seshachalam-yv
This commit introduces a new Makefile target `make deploy-via-kusto…
231818f 
…mize` to support kustomize-based deployments.
@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 May 30, 2023
@seshachalam-yv
feat: Improve make kind-up with instruction to set KUBECONFIG
a6cf748 
This commit enhances the `make kind-up` command by adding a console message that instructs the user to set the KUBECONFIG environment variable for the newly created KinD cluster.

``` bash
📌 NOTE: To target the newly created KinD cluster, please run the following command:

    export KUBECONFIG=/Users/I568019/work/etcd-druid/hack/e2e-test/infrastructure/kind/kubeconfig
```
@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 May 30, 2023
@seshachalam-yv
Copy link
Contributor Author

seshachalam-yv commented May 30, 2023 •
edited
Loading

@seshachalam-yv thanks for the PR. Overall LGTM.

  1. Can you please add this line to Makefile target make kind-up to make it easy for developers to set the KUBECONFIG of the new KinD cluster before they run make deploy?
@echo -e "Please run the following command to target the newly created KinD cluster: \n export KUBECONFIG=hack/e2e-test/infrastructure/kind/kubeconfig"

Fixed as part of a6cf748

Can you also please add a release note, since this is a breaking change for developers who are actively using make deploy who will now need to move to `make deploy-via-kustomize? Thanks.

Updated release note.

@seshachalam-yv seshachalam-yv self-assigned this May 30, 2023
@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 May 30, 2023
@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 May 30, 2023
unmarshall
unmarshall reviewed May 30, 2023
View reviewed changes
Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
shreyas-s-rao
shreyas-s-rao approved these changes May 30, 2023
View reviewed changes
Copy link
Contributor

@shreyas-s-rao shreyas-s-rao left a comment

Choose a reason for hiding this comment

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

@seshachalam-yv thanks for addressing my comments.

/lgtm

unmarshall
unmarshall approved these changes May 30, 2023
View reviewed changes
Copy link
Contributor

@unmarshall unmarshall left a comment

Choose a reason for hiding this comment

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

/lgtm

@seshachalam-yv seshachalam-yv merged commit b07c666 into gardener:master May 31, 2023
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label May 31, 2023
@seshachalam-yv seshachalam-yv deleted the update-makefile-skaffold-deploy branch May 31, 2023 04:33
@gardener-robot gardener-robot removed the status/accepted Issue was accepted as something we need to work on label May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@unmarshall unmarshall unmarshall approved these changes

@abdasgupta abdasgupta abdasgupta approved these changes

@shreyas-s-rao shreyas-s-rao shreyas-s-rao approved these changes

Assignees

@shreyas-s-rao shreyas-s-rao

Labels
area/dev-productivity Developer productivity related (how to improve development) kind/enhancement Enhancement, improvement, extension needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) reviewed/lgtm Has approval for merging size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) status/closed Issue is closed (either delivered or triaged)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@seshachalam-yv @abdasgupta @unmarshall @shreyas-s-rao @gardener-robot-ci-1 @gardener-robot-ci-2 @gardener-robot-ci-3 @gardener-robot