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-2696] Appoint specific version when installing yunikorn #452

Closed
wants to merge 3 commits into from

Conversation

Paul-Lyu
Copy link
Contributor

@Paul-Lyu Paul-Lyu commented Jul 8, 2024

What is this PR for?

Appoint specific version when installing yunikorn
In get started doc, image tags are latest which is not available on docker hub.

What type of PR is it?

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

Todos

  • - Task

What is the Jira issue?

https://issues.apache.org/jira/browse/YUNIKORN-2696

How should this be tested?

Screenshots (if appropriate)

Questions:

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

@chia7712 chia7712 requested a review from 0yukali0 July 8, 2024 10:07
chia7712

This comment was marked as outdated.

Copy link
Contributor

@craigcondit craigcondit left a comment

Choose a reason for hiding this comment

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

I don't think we should hardcode a specific version here. This will get out-of-date quickly. Instead, reference the tags as "scheduler-{version}" and point users to DockerHub for available tags.

Also, drop reference to 'latest'. 'docker hub' should be 'DockerHub', and 'Yunikorn' should be 'YuniKorn'.

Copy link
Contributor

@craigcondit craigcondit left a comment

Choose a reason for hiding this comment

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

See previous comment.

@chia7712
Copy link
Member

I don't think we should hardcode a specific version here. This will get out-of-date quickly. Instead, reference the tags as "scheduler-{version}" and point users to DockerHub for available tags.

agree. This PR can align the example of https://github.com/apache/yunikorn-site/blob/master/docs/user_guide/service_config.md?plain=1#L39

'Yunikorn' should be 'YuniKorn'.

There are many such typo in code base. Maybe we should fix it. at least yunikorn-site repo

@Paul-Lyu Paul-Lyu changed the title Appoint specific version when installing yunikorn [YUNIKORN-2696] Appoint specific version when installing yunikorn Jul 18, 2024
@ryankert01
Copy link
Contributor

I don't think we should hardcode a specific version here. This will get out-of-date quickly. Instead, reference the tags as "scheduler-{version}" and point users to DockerHub for available tags.

agree. This PR can align the example of https://github.com/apache/yunikorn-site/blob/master/docs/user_guide/service_config.md?plain=1#L39

I agree; we can align this method with all versioned examples in the docs.

Copy link
Contributor

@wilfred-s wilfred-s left a comment

Choose a reason for hiding this comment

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

LGTM, I think all requested changes are done

@wilfred-s wilfred-s requested a review from craigcondit July 30, 2024 02:11
Copy link
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

@chenyulin0719
Copy link
Contributor

chenyulin0719 commented Aug 19, 2024

Sorry for the late review.
Instead of providing a custom.yml, I would prefer to use a single command. (Refer the chart configuration list)

EX:

VERSION=1.5.2
helm install yunikorn yunikorn/yunikorn --namespace yunikorn \
  --set image.tag=scheduler-${VERSION} \
  --set pluginImage.tag=scheduler-plugin-${VERSION} 
  --set admissionController.image.tag=admission-${VERSION} \
  --set web.image.tag=web-${VERSION}

The even better solution is to extract variable version in helm's values.yml, so we could use:

VERSION=1.5.2
helm install yunikorn yunikorn/yunikorn --namespace yunikorn \
  --set version=${VERSION}

If anyone agree with me, we can create a new Jira for the improvement.

Copy link
Contributor

@chenyulin0719 chenyulin0719 left a comment

Choose a reason for hiding this comment

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

LGTM. Since this PR already addresses the request, I will merge it later today.

@blueBlue0102
Copy link
Contributor

Sorry for the late review. Instead of providing a custom.yml, I would prefer to use a single command. (Refer the chart configuration list)
...
...
The even better solution is to extract variable version in helm's values.yml, so we could use:

VERSION=1.5.2
helm install yunikorn yunikorn/yunikorn --namespace yunikorn \
  --set version=${VERSION}

If anyone agree with me, we can create a new Jira for the improvement.

I agree with @chenyulin0719; using a single command is more convenient than providing a custom.yml.

@chia7712
Copy link
Member

using a single command is more convenient than providing a custom.yml.

sounds good to me

@chenyulin0719
Copy link
Contributor

Created the followup Jira: [YUNIKORN-2819] Add version parameter to Helm chart

github-actions bot pushed a commit that referenced this pull request Aug 20, 2024
github-actions bot pushed a commit to manirajv06/incubator-yunikorn-site that referenced this pull request Aug 22, 2024
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.

7 participants