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

feat: worklow: add permissions field #2312

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Frankkkkk
Copy link

The permissions field can either be a top level key, or a per-job one. It supports a map of scope<>permission relations.
For example, to allow the jobs to write to the the packages registry, the following must be set:

permissions:
  packages: write

this fixes that.
Right now, this field is not used/useful when running the actions locally, but can be used to handle permissions on forges that implement the same action format, such as Forgejo/Gitea.

The `permissions` field can either be a top level key, or a per-job one.
It supports a map of scope<>permission relations.
For example, to allow the jobs to write to the the packages registry,
the following must be set:
```yaml
permissions:
  packages: write
```

this fixes that.
Right now, this field is not used/useful when running the actions
locally, but can be used to handle permissions on forges that implement the
same action format, such as Forgejo/Gitea.

Signed-off-by: Frank Villaro-Dixon <[email protected]>
@Frankkkkk Frankkkkk requested a review from a team as a code owner May 1, 2024 21:00
@ChristopherHX
Copy link
Contributor

Notice: permissions: write-all and permissions: read-all are valid as well, but cannot be parsed to a map.

@Frankkkkk
Copy link
Author

Frankkkkk commented May 1, 2024

Good catch, indeed! What do you propose? Would having something like:

type Permission interface {}
...
Permissions Permission `yaml:"permissions"`

be better? Thanks!

@ChristopherHX
Copy link
Contributor

I usually use yaml.Node as the type, since this allows to call decode to both string and map

However I'm not shure how the rest of the code is it doing here...

@ChristopherHX
Copy link
Contributor

Also check the gitea/act repo, and you will see this struct is duplicated there into another package on their standalone workflow parser

Copy link
Contributor

@ChristopherHX ChristopherHX left a comment

Choose a reason for hiding this comment

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

Like a months ago, I can't agree merging this as long it breaks parsing working workflows that can be used today without adding a meaningful feature to act.

Implementing the following for the permission map can also map read-all, write-all
https://github.com/go-yaml/yaml/blob/v3.0.1/yaml.go#L36
e.g. node.Decode(self) and if it returns an error try to match for string constants or return an error

@ChristopherHX
Copy link
Contributor

ChristopherHX commented Jun 5, 2024

Oh we are still using yaml v2, where it is a different unmarshallyaml interface. Maybe I should consider to upgrade the whole repo to v3

edit hmm all weird v3 had v2 backcompat for that interface, we are v3 already

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants