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

✨ (kustomize/v2) Remove deprecated syntax usage by replacing patchesStrategicMerge with patches #3374

Merged
merged 1 commit into from
May 11, 2023

Conversation

Sajiyah-Salat
Copy link
Contributor

Fixes #3372

See that the default scaffolds:

Warning:
'patchesStrategicMerge' is deprecated. Please use 'patches' instead. Run
"kustomize edit fix' to update
your Kustomization automatically.

Therefore I replaced patchesStrategicMerge by patches in the kustomize/v2 templates code.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 30, 2023
@Sajiyah-Salat
Copy link
Contributor Author

I have run make generate. why its failing?

@@ -110,7 +110,7 @@ var kustomizationTemplate = `# This kustomization.yaml is not intended to be run
resources:
%s

patchesStrategicMerge:
patches:
Copy link
Member

@NikhilSharmaWe NikhilSharmaWe Apr 30, 2023

Choose a reason for hiding this comment

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

@Sajiyah-Salat Try to make this change again in an up-to-date branch (upstream/master) and run make generate. After running make generate you will see that these files are also updated:

  • docs/book/src/component-config-tutorial/testdata/project/config/crd/kustomization.yaml
  • pkg/plugins/common/kustomize/v2/scaffolds/internal/templates/config/crd/kustomization.go
  • testdata/project-v4-config/config/crd/kustomization.yaml
  • testdata/project-v4-declarative-v1/config/crd/kustomization.yaml
  • testdata/project-v4-multigroup/config/crd/kustomization.yaml
  • testdata/project-v4-with-deploy-image/config/crd/kustomization.yaml
  • testdata/project-v4/config/crd/kustomization.yaml

then commit the changes.

Not sure why oryxBuildBinary is added, so lets try it again.

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 2, 2023
@NikhilSharmaWe
Copy link
Member

@Sajiyah-Salat It seems fine now.
Could you please squash the commits?

@Sajiyah-Salat
Copy link
Contributor Author

$git push
To https://github.com/Sajiyah-Salat/kubebuilder.git
 ! [rejected]          sajiyah#3372 -> sajiyah#3372 (non-fast-forward)
error: failed to push some refs to 'https://github.com/Sajiyah-Salat/kubebuilder.git'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Integrate the remote changes (e.g.
hint: 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

squashing always comes with frustration. I have squashed the commits. i have tried pulling as well. still its giving the following error

@NikhilSharmaWe
Copy link
Member

NikhilSharmaWe commented May 2, 2023

@Sajiyah-Salat This might be because your local branch is not up to date with the origin branch.

Try this:

  1. git fetch origin
  2. git reset --hard origin/sajiyah#3372
  3. git reset commitID-of-last-commit-before-yours
  4. git add .
  5. git commit -m "patchesStrategicMerge to patches"

@NikhilSharmaWe
Copy link
Member

NikhilSharmaWe commented May 3, 2023

/approve

Copy link
Member

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution.

  • Please, remove oryxBuildBinary
  • Also, after the change in the boilerplate, the samples under testdata and docs should be automatically updated by running make generate
  • Also, we have a standard for the titles because they will be used to generate the release notes. So, in this case I am changing the title of this PR accordingly so that you can better check it and I hope that you do not mind.

Note Details about this review: https://github.com/kubernetes-sigs/kubebuilder/pull/3374/files#r1181174158

Please, ensure that you read the Contributing to know how to contribute with the project. Also, feel free to contribute to this doc if you see that it is missing or has outdated info.

@camilamacedo86 camilamacedo86 changed the title 🐛 patchesStrategicMerge to patches. ✨ : (kustomize/v2) Solve deprecation syntax usage by replacing patchesStrategicMerge with patches May 4, 2023
@camilamacedo86 camilamacedo86 changed the title ✨ : (kustomize/v2) Solve deprecation syntax usage by replacing patchesStrategicMerge with patches ✨ (kustomize/v2) Remove deprecated syntax usage by replacing patchesStrategicMerge with patches May 4, 2023
camilamacedo86
camilamacedo86 previously approved these changes May 8, 2023
Copy link
Member

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

That is great 🥇
Thank you for the contribution 🚀

See that testdata CI is falling because it seems that docs are not successfully update.
Note that:

Also, ensure that you have your branch updated (rebased) with the master.

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 8, 2023
@camilamacedo86 camilamacedo86 dismissed their stale review May 8, 2023 09:36

Missing properly update the testdata

@k8s-ci-robot k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 8, 2023
@camilamacedo86 camilamacedo86 removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 8, 2023
@Sajiyah-Salat
Copy link
Contributor Author

Sajiyah-Salat commented May 8, 2023

The mistake I did is I deleted oryxBuildBinary and directly pushed it to branch without running make generate. Now when I run make generate and git status it says everything is upto date. What to do?
About the env: I use codespaces if needed as my most of the work is in docs part.

@lauchokyip
Copy link
Contributor

lauchokyip commented May 9, 2023

@Sajiyah-Salat This might be because your local branch is not up to date with the origin branch.

Try this:

  1. git fetch origin
  2. git reset --hard origin/sajiyah#3372
  3. git reset commitID-of-last-commit-before-yours
  4. git add .
  5. git commit -m "patchesStrategicMerge to patches"

@NikhilSharmaWe git reset --hard is not a good practice and should be avoided because it will erase your local history
@Sajiyah-Salat Please checkout this guide to learn about Git Workflow (https://www.kubernetes.dev/docs/guide/github-workflow/)

@NikhilSharmaWe
Copy link
Member

@lauchokyip Yes, I understand that but just wanted @Sajiyah-Salat to make the correct changes with simplicity, just for this case.

@Sajiyah-Salat Sajiyah-Salat force-pushed the sajiyah#3372 branch 2 times, most recently from 846d8cf to a50259c Compare May 10, 2023 13:49
@Sajiyah-Salat
Copy link
Contributor Author

I think the tests have some issues. I have run make generate. Following files suggested by @NikhilSharmaWe Changed as well. Still its failing. Why?

@camilamacedo86
Copy link
Member

HI @Sajiyah-Salat,

Did you have your branch rebased with master?
Can you try:

1 _ update the master branch

$ git checkout master
$ git pull upstream master # I do not know how did you set the git to looking for the remote source. I use upstream

2 - rebase with your branch

$ git checkout
$ git rebase master

3 - run make install # It will ensure that you have the bin with the source code changes
4 - run make generate # It will generate the samples under testdata and docs

Copy link
Member

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

Great @Sajiyah-Salat !!!!

/approved
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 11, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, NikhilSharmaWe, Sajiyah-Salat

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 11, 2023
@Sajiyah-Salat
Copy link
Contributor Author

wooh.... after a long time. Thank you for guiding me through out.

@k8s-ci-robot k8s-ci-robot merged commit 77bc895 into kubernetes-sigs:master May 11, 2023
mjlshen added a commit to mjlshen/kubebuilder that referenced this pull request Jun 14, 2023
This commit fixes the syntax error introduced in kubernetes-sigs#3374 where
pathcesStrategicMerge was replaced with patches, which now requires that
every patch additionally have a "path" key when multiple patches are
specified in a file.

Signed-off-by: Michael Shen <mishen@umich.edu>
mjlshen added a commit to mjlshen/kubebuilder that referenced this pull request Jun 14, 2023
This commit fixes the syntax error introduced in kubernetes-sigs#3374 where
pathcesStrategicMerge was replaced with patches, which now requires that
every patch additionally have a "path" key when multiple patches are
specified in a file.

Signed-off-by: Michael Shen <mishen@umich.edu>
mjlshen added a commit to mjlshen/kubebuilder that referenced this pull request Jun 14, 2023
This commit fixes the syntax error introduced in kubernetes-sigs#3374 where
pathcesStrategicMerge was replaced with patches, which now requires that
every patch additionally have a "path" key when multiple patches are
specified in a file.

Signed-off-by: Michael Shen <mishen@umich.edu>
mjlshen added a commit to mjlshen/kubebuilder that referenced this pull request Jun 14, 2023
This commit fixes the syntax error introduced in kubernetes-sigs#3374 where
pathcesStrategicMerge was replaced with patches, which now requires that
every patch additionally have a "path" key when multiple patches are
specified in a file.

Signed-off-by: Michael Shen <mishen@umich.edu>
mjlshen added a commit to mjlshen/kubebuilder that referenced this pull request Jun 20, 2023
This commit fixes the syntax error introduced in kubernetes-sigs#3374 where
pathcesStrategicMerge was replaced with patches, which now requires that
every patch additionally have a "path" key when multiple patches are
specified in a file.

Signed-off-by: Michael Shen <mishen@umich.edu>
mjlshen added a commit to mjlshen/kubebuilder that referenced this pull request Jun 20, 2023
This commit fixes the syntax error introduced in kubernetes-sigs#3374 where
pathcesStrategicMerge was replaced with patches, which now requires that
every patch additionally have a "path" key when multiple patches are
specified in a file.

Signed-off-by: Michael Shen <mishen@umich.edu>
mjlshen added a commit to mjlshen/kubebuilder that referenced this pull request Jun 20, 2023
This commit fixes the syntax error introduced in kubernetes-sigs#3374 where
pathcesStrategicMerge was replaced with patches, which now requires that
every patch additionally have a "path" key when multiple patches are
specified in a file.

Signed-off-by: Michael Shen <mishen@umich.edu>
mjlshen added a commit to mjlshen/kubebuilder that referenced this pull request Jun 21, 2023
This commit fixes the syntax error introduced in kubernetes-sigs#3374 where
pathcesStrategicMerge was replaced with patches, which now requires that
every patch additionally have a "path" key when multiple patches are
specified in a file.

Signed-off-by: Michael Shen <mishen@umich.edu>
mjlshen added a commit to mjlshen/kubebuilder that referenced this pull request Jun 21, 2023
This commit fixes the syntax error introduced in kubernetes-sigs#3374 where
pathcesStrategicMerge was replaced with patches, which now requires that
every patch additionally have a "path" key when multiple patches are
specified in a file.

Signed-off-by: Michael Shen <mishen@umich.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(kustomize/v2) - Remove the deprecation
5 participants