Uh oh!
There was an error while loading. Please reload this page.
Specify schema for resource permissions - #2674
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Copilot reviewed 29 out of 29 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
bundle/config/resources/pipeline.go:13
- [nitpick] Consider defining constants for PipelinePermissionLevel (similar to those in job.go) to promote consistency and help prevent typos when specifying permission levels for pipelines.
type PipelinePermissionLevel string
pietern
reviewed
Apr 8, 2025
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| import "fmt" | ||
| // Permission holds the permission level setting for a single principal. | ||
| // Multiple of these can be defined on any resource. |
Contributor
There was a problem hiding this comment.
Is this generic permission struct still used?
CollaboratorAuthor
There was a problem hiding this comment.
Yes, we need that for top-level permissions. We can make it enum as well, but it needs more thought
kanterov
marked this pull request as ready for review
April 8, 2025 18:01
kanterov
requested review from
andrewnester, anton-107, denik and shreyas-goenka
as code ownersApril 8, 2025 18:01
kanterov
commented
Apr 8, 2025
CollaboratorAuthor
@pietern fixed, please take a look again |
pietern
approved these changes
Apr 9, 2025
pietern
left a comment
Contributor
There was a problem hiding this comment.
Thanks!
It would be nice to have native enum support so that we can issue errors if a user specifies an unsupported value. With schema support alone, it will produce warnings in an editor, but the CLI still happily accepts invalid values.
deco-sdk-taggingBot
added a commit
that referenced
this pull request
Apr 9, 2025
## Release v0.247.0 ### CLI * Added include/exclude flags support to sync command ([#2650](#2650)) ### Bundles * Added support for model serving endpoints in deployment bind/unbind commands ([#2634](#2634)) * Added include/exclude flags support to bundle sync command ([#2650](#2650)) * Added JSON schema for resource permissions ([#2674](#2674)) * Removed pipeline 'deployment' field from jsonschema ([#2653](#2653)) * Updated JSON schema for deprecated pipeline fields ([#2646](#2646)) * The --config-dir and --source-dir flags for "bundle generate app" are now relative to CWD, not bundle root ([#2683](#2683)) * Reverts [#2549](#2549) to resolve issues with Web Terminal host mismatch ([#2685](#2685))
denik pushed a commit
that referenced
this pull request
May 20, 2026
## Changes Specify JSON schema for resource permissions. We replace a string with an enum for the `level` field. Each resource supports different permission-level values and has its own enum. Bundle root permissions remain as-is, and there is already a translation code that, for example, changes `CAN_RUN` into `CAN_MANAGE_RUN` for jobs. However, when `CAN_RUN` is specified directly on a job, it doesn't get translated, which makes sense. With the correct schema, we are going to avoid the mistake of accidentally specifying `CAN_RUN`, assuming that it works. ## Why `resources.Permission` schema allows arbitrary strings while different resources support different permission levels. ## Tests Manually by inspecting JSON schema
denik pushed a commit
that referenced
this pull request
May 20, 2026
## Release v0.247.0 ### CLI * Added include/exclude flags support to sync command ([#2650](#2650)) ### Bundles * Added support for model serving endpoints in deployment bind/unbind commands ([#2634](#2634)) * Added include/exclude flags support to bundle sync command ([#2650](#2650)) * Added JSON schema for resource permissions ([#2674](#2674)) * Removed pipeline 'deployment' field from jsonschema ([#2653](#2653)) * Updated JSON schema for deprecated pipeline fields ([#2646](#2646)) * The --config-dir and --source-dir flags for "bundle generate app" are now relative to CWD, not bundle root ([#2683](#2683)) * Reverts [#2549](#2549) to resolve issues with Web Terminal host mismatch ([#2685](#2685))
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
Specify JSON schema for resource permissions.
We replace a string with an enum for the
levelfield. Each resource supports different permission-level values and has its own enum.Bundle root permissions remain as-is, and there is already a translation code that, for example, changes
CAN_RUNintoCAN_MANAGE_RUNfor jobs. However, whenCAN_RUNis specified directly on a job, it doesn't get translated, which makes sense. With the correct schema, we are going to avoid the mistake of accidentally specifyingCAN_RUN, assuming that it works.Why
resources.Permissionschema allows arbitrary strings while different resources support different permission levels.Tests
Manually by inspecting JSON schema