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

Remove unnecessary kustomize in make helm #1370

Merged
merged 1 commit into from
Aug 31, 2023
Merged

Remove unnecessary kustomize in make helm #1370

merged 1 commit into from
Aug 31, 2023

Conversation

shubhscoder
Copy link
Contributor

@shubhscoder shubhscoder commented Aug 29, 2023 •
edited
Loading

Why are these changes needed?

We first make kustomize as part of make helm (and hence make sync). But this is not required. This was realized while running make sync. This code change simply removes kustomize from make helm.

Related issue number

Closes #1368

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • This PR is not tested :(

@shubhscoder
Copy link
Contributor Author

@kevin85421 This is the PR for 1368. I don't seem to have access to assignees section to add you as the reviewer. Thanks!

@kevin85421
Copy link
Member

Could you remove kustomize from helm in the Makefile as I mentioned in #1368 (comment)? Additionally, would you test all the kustomize-related targets listed in the Makefile? In addition, are there any breaking changes between 3.10.0 and 5.1.1?

@shubhscoder
Copy link
Contributor Author

@kevin85421 Apologies for missing the helm part. I have updated the code changes.
Yes sure, I will test all the targets locally.
Also, let me investigate if there are any breaking changes from 3.10.0 to 5.1.1

I will comment on this PR once I am done with the testing and the investigation. Thanks for the review.

blublinsky
blublinsky approved these changes Aug 30, 2023
View reviewed changes
Copy link
Contributor

@blublinsky blublinsky left a comment

Choose a reason for hiding this comment

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

LGTM

ray-operator/Makefile Outdated
@@ -114,7 +114,7 @@ controller-gen: ## Download controller-gen locally if necessary.

KUSTOMIZE = $(shell pwd)/bin/kustomize
kustomize: ## Download kustomize locally if necessary.
test -s $(KUSTOMIZE) || GOBIN=$(KUSTOMIZE)/.. go install sigs.k8s.io/kustomize/kustomize/[email protected]
test -s $(KUSTOMIZE) || GOBIN=$(KUSTOMIZE)/.. go install sigs.k8s.io/kustomize/kustomize/[email protected]
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to just use latest?

Copy link
Contributor Author

@shubhscoder shubhscoder Aug 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Got it!

@shubhscoder
Copy link
Contributor Author

@kevin85421 I tried the targets using Kustomize and they are building fine on my machine.
However, there are breaking changes that are introduced in [email protected] as seen here.
Therefore, I will try to investigate what of the breaking changes made are relevant to kuberay.

Also, the kustomize version being used in apiserver is 3.8.7, should we upgrade that as well?

@shubhscoder
Copy link
Contributor Author

Closing this as the issue was fixed as part of e7fbf7d

@shubhscoder shubhscoder reopened this Aug 31, 2023
@kevin85421
Copy link
Member

Also, the kustomize version being used in apiserver is 3.8.7, should we upgrade that as well?

^ cc @blublinsky

@shubhscoder
Copy link
Contributor Author

Also, the kustomize version being used in apiserver is 3.8.7, should we upgrade that as well?

^ cc @blublinsky

If needed, I can create a seperate PR for that. Since this one was open, I removed the unnecessary kustomize in make helm

@shubhscoder shubhscoder changed the title Upgrade kustomize version to 5.1.1 from 3.10.0 Remove unnecessary kustomize in make helm Aug 31, 2023
@blublinsky
Copy link
Contributor

Yes, I think we should upgrade kustomize throughout the project to the same version

@kevin85421 kevin85421 merged commit cc2e144 into ray-project:master Aug 31, 2023
@shubhscoder
Copy link
Contributor Author

Yes, I think we should upgrade kustomize throughout the project to the same version

Thanks! I will create an issue and raise a CR to make kustomize version consistent across the project.

lowang-bh pushed a commit to lowang-bh/kuberay that referenced this pull request Sep 24, 2023
@shubhscoder
Remove kustomize from helm, as it is not required (ray-project#1370)
d7b4473 
Remove kustomize from helm, as it is not required
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blublinsky blublinsky blublinsky approved these changes

@kevin85421 kevin85421 kevin85421 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug] make sync fails locally
3 participants
@shubhscoder @kevin85421 @blublinsky