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

Introduce CEL based validation in App spec for SA and Cluster #1380

Open
varshaprasad96 opened this issue Oct 26, 2023 · 3 comments · May be fixed by #1382
Open

Introduce CEL based validation in App spec for SA and Cluster #1380

varshaprasad96 opened this issue Oct 26, 2023 · 3 comments · May be fixed by #1382
Assignees
Labels
carvel-accepted This issue should be considered for future work and that the triage process has been completed enhancement This issue is a feature request

Comments

@varshaprasad96
Copy link

Describe the problem/challenge you have

Tried creating an App CR with no service account or cluster specified. The resource got created, but reconcile errored stating:
Reconcile failed: Preparing kapp: Expected service account or cluster specified. The documentation states that both of them are optional fields, hence as a user I'm unaware that one of them needs to be specified.

Describe the solution you'd like

Based on the conversation here, it does make sense to either have the user specify the target cluster where App CR needs to be installed, or the sa which needs to be used to install it in the same cluster. However, running this validation after the resource is created is not necessary (refer:

return AccessInfo{}, fmt.Errorf("Expected service account or cluster specified")
). This can also be done on the admission control level before an App CR is created.

Solution: Add CEL based validation to ensure that one of them are specified.

Anything else you would like to add:

None


Vote on this request

This is an invitation to the community to vote on issues, to help us prioritize our backlog. Use the "smiley face" up to the right of this comment to vote.

👍 "I would like to see this addressed as soon as possible"
👎 "There are other more important things to focus on right now"

We are also happy to receive and review Pull Requests if you want to help working on this issue.

@varshaprasad96 varshaprasad96 added carvel-triage This issue has not yet been reviewed for validity enhancement This issue is a feature request labels Oct 26, 2023
@prembhaskal prembhaskal self-assigned this Oct 27, 2023
@varshaprasad96 varshaprasad96 linked a pull request Oct 27, 2023 that will close this issue
5 tasks
@varshaprasad96
Copy link
Author

@prembhaskal Sorry, I missed that you had assigned this issue. Could you please help in reviewing the PR? Thank you!

@prembhaskal
Copy link
Member

Thank you for reporting the issue. I am checking the PR.

@prembhaskal prembhaskal moved this to Unprioritized in Carvel Nov 7, 2023
@prembhaskal prembhaskal moved this from Unprioritized to To Triage in Carvel Nov 7, 2023
@prembhaskal prembhaskal added carvel-accepted This issue should be considered for future work and that the triage process has been completed and removed carvel-triage This issue has not yet been reviewed for validity labels Nov 7, 2023
@prembhaskal prembhaskal moved this from To Triage to Unprioritized in Carvel Nov 7, 2023
@GrahamDumpleton
Copy link

Old related issue: #598

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
carvel-accepted This issue should be considered for future work and that the triage process has been completed enhancement This issue is a feature request
Projects
Status: Unprioritized
Development

Successfully merging a pull request may close this issue.

3 participants