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-2903]modify Dev Environment Setup ServiceAccount token generate guide #492

Closed
wants to merge 7 commits into from

Conversation

xchwan
Copy link

@xchwan xchwan commented Oct 17, 2024

What is this PR for?

modify k8s dashboard token generation part. According to https://stackoverflow.com/questions/76133984/kubernetes-service-account-doesnt-have-token, k8s service account don't generate token by default v1.24

What type of PR is it?

  • - Improvement

What is the Jira issue?

YUNIKORN-2903

Copy link
Contributor

@ryankert01 ryankert01 left a comment

Choose a reason for hiding this comment

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

Overall lgtm, some minors. We can eliminate 3. actually.

docs/developer_guide/env_setup.md Outdated Show resolved Hide resolved
Copy link
Contributor

@ryankert01 ryankert01 left a comment

Choose a reason for hiding this comment

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

lgtm

@ryankert01
Copy link
Contributor

Please at least cover our latest v1.6.0 versioned docs also.

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.

By default, the default service account has no permission to view pods.
The developer must bind cluster role to the account, please check below document.

We could just put the link as reference.

@xchwan xchwan requested a review from chenyulin0719 October 19, 2024 13:23
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.

One comment left below. And could you please update v1.6.0 versioned docs too. It would be helpful for the new developers.

docs/developer_guide/env_setup.md Outdated Show resolved Hide resolved
@xchwan
Copy link
Author

xchwan commented Oct 20, 2024

Please at least cover our latest v1.6.0 versioned docs also.

done, pls check

Copy link
Contributor

@ryankert01 ryankert01 left a comment

Choose a reason for hiding this comment

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

please also update assets

@xchwan
Copy link
Author

xchwan commented Oct 20, 2024

update assets ok

Copy link
Contributor

@ryankert01 ryankert01 left a comment

Choose a reason for hiding this comment

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

lgtm

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.

+1 LGTM

github-actions bot pushed a commit that referenced this pull request Oct 21, 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.

3 participants