-
Notifications
You must be signed in to change notification settings - Fork 170
fix: add applySas #432
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
fix: add applySas #432
Conversation
Signed-off-by: LY-today <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: LY-today The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Hi @LY-today. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Signed-off-by: LY-today <[email protected]>
sanposhiho
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/ok-to-test
/cc @saza-ku @ordovicia @utam0k
|
@sanposhiho: GitHub didn't allow me to request PR reviews from the following users: saza-ku, ordovicia. Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Signed-off-by: LY-today <[email protected]>
saza-ku
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add test cases of importing/syncing pods with account services?
|
|
||
| // ResourcesForLoad indicates all resources and scheduler configuration to be loaded. | ||
| type ResourcesForLoad struct { | ||
| Sas []v1.ServiceAccountApplyConfiguration `json:"sas"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now SnapshotService is only used by SnapshotHandler and it only handles resources of the fake cluster. So we don't need to change SnapshotService.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I need to delete this line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@saza-ku Any suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean we don't need the entire fix in SnapshotService, not only the line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of the DefaultGVRs of simulator/oneshotimporter/importer.go
No, I mean we need to fix both oneshotimporter and syncer. And this has been achived by 8bab17c. So that's okay!
And I mistakenly believed that SnapshotService didn't need to handle ServiceAccount. But if you export resources that is imported by real clusters, it can contain ServiceAccount that is other than default. Sorry for my misunderstanding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we need additional fix. It needs to Snap ServiceAccount not only Load.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@saza-ku How can I fix it? Currently, all the changes in this PR can run normally on my local machine and meet my needs. Other fixes may require your guidance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost the same as Load. You would add Sas to ResourcesForSnap, and implement listSas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost the same as
Load. You would addSastoResourcesForSnap, and implementlistSas.
done
|
Does this error occur when you run |
Signed-off-by: LY-today <[email protected]>
What is your go version? |
@saza-ku |
Signed-off-by: LY-today <[email protected]>
|
@LY-today: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@saza-ku ci reports this error, how can I adjust it? |
|
cyclop calculates complexity in this way: https://github.com/bkielbasa/cyclop/blob/master/pkg/analyzer/analyzer.go |
|
We don't care about to ServiceAccount. |
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
|
@k8s-triage-robot: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |


#421