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

[YUNIKORN-1779] Deduplicate the deployment files #679

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

targetoee
Copy link
Contributor

@targetoee targetoee commented Sep 16, 2023

What is this PR for?

The release repo uses helm chart for deployment in the e2e tests. These files (in k8shim repo & release repo) are nearly identical. We should not maintain duplicate copies of that.

What type of PR is it?

  • - Bug Fix
  • - Improvement
  • - Feature
  • - Documentation
  • - Hot Fix
  • - Refactoring

Todos

What is the Jira issue?

https://issues.apache.org/jira/projects/YUNIKORN/issues/YUNIKORN-1779

How should this be tested?

Screenshots (if appropriate)

Questions:

  • - The licenses files need update.
  • - There is breaking changes for older versions.
  • - It needs documentation.

@steinsgateted
Copy link
Contributor

steinsgateted commented Sep 16, 2023

Hello @targetoee @wilfred-s,
I would like some clarifications.Thank you.
I don't think we need to delete all these examples.
Users don’t need to know how to use helm, and they can also apply file by file.
Like in the documentation https://yunikorn.apache.org/docs/developer_guide/deployment
Step by step instructions to learn how to deploy yunikorn.
I created two PRs to see if they make sense. Thanks. #678 YUNIKORN-1955

@targetoee targetoee changed the title [HDDS-1779] Deduplicate the deployment files [YUNIKORN-1779] Deduplicate the deployment files Sep 16, 2023
@steinsgateted steinsgateted self-requested a review September 17, 2023 14:01
@targetoee
Copy link
Contributor Author

Hi @steinsgateted, thanks for the advice.
I understand that removing these files might cause some inconvenience.
One approach we could consider is moving these files into the release repo and generating them using helm template along with a script (for removing helm-related items).
This way, we can keep these files in sync and provide some convenience for users.
Does this make sense to you?

@targetoee
Copy link
Contributor Author

Hi @wilfred-s, what do you think about these two methods: just removing duplicate files, or keeping them and using some way to sync them?

@codecov
Copy link

codecov bot commented Sep 25, 2023

Codecov Report

Merging #679 (f44f624) into master (8b26c37) will increase coverage by 0.02%.
Report is 13 commits behind head on master.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #679      +/-   ##
==========================================
+ Coverage   71.87%   71.90%   +0.02%     
==========================================
  Files          51       51              
  Lines        8079     8082       +3     
==========================================
+ Hits         5807     5811       +4     
- Misses       2074     2075       +1     
+ Partials      198      196       -2     

see 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@targetoee
Copy link
Contributor Author

Hi @steinsgateted and @wilfred-s, I have added a script to the yunikorn-release repository that can generate deployment files using helm template. Could this be a solution for the issue?

related PRs:
#350
#157

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