Open dvjn opened 4 months ago
Hi @6543 @anbraten @qwerty287 I have done initial UI changes for this, please review the proposal and screenshots and let me know if I should continue working on this and implement the rest.
Screenshots:
Repo Settings:
User Settings:
Admin Settings:
Add Variable Screen:
As variables are not secret anyways there was a suggestion somewhere to have a special config file like .woodpecker/config.yaml
in the repo which can be used for such settings. This would be quite handy as it would also allow to use those variables in the cli exec
command as well and allows easy copies of that config to other woodpecker instances etc. Should those variables then be shown in the UI as well as read-only or should only the config file be the source of them? What do you think?
should only the config file be the source of them
I'd prefer this way, but it's not that easy to have a good way to implement this at org/global level.
As variables are not secret anyways there was a suggestion somewhere to have a special config file like
.woodpecker/config.yaml
in the repo which can be used for such settings.
I think this is not as useful as it seems, since you can already use variables in a workflow file. This config.yaml
approach is mainly beneficial when you need these variables in multiple workflows within the same repository.
However, the most important use-cases of this PR, in my opinion, are: first, the ability to manage global variables using the admin UI, and second, the capability to share variables across repositories under an organization or user.
FYI, the solution I'm proposing is heavily inspired by Gitea.
@anbraten @qwerty287 @zc-devs Do let me know your final thoughts on this and let me know if I should move forward with implementing it.
I can also add agent variables to address #3311
your final thoughts
I like the idea.
if I should move forward with implementing it
I am not a right guy :) Let's hear from maintainers.
I can also add agent variables
That would be great!
Yes, the way you proposed would be fine for me.
However, you write "Fixes #3661" in the desc which is not correct (I believe there's an issue about your thing too). #3661 is about workflow-level vars and this should definitely be defined in the yaml.
However, you write "Fixes #3661" in the desc which is not correct (I believe there's an issue about your thing too). #3661 is about workflow-level vars and this should definitely be defined in the yaml.
My bad, will remove that issue from the description.
I'm really interested in this PR. Keep moving! Thanks for your work.
I have completed the code changes in the server and web frontend. This means the feature is now in a fully usable state. The docs and cli changes are still pending. Planning to do those after an initial round of code review.
It would be great if someone could review the code.
Summary of Changes:
from_variable
and variables
(similar to from_secret
and secrets
) in pipeline config.variables
, with migrations.Wasn't there an idea to drop secrets
section from step? Maybe let's do not add variables section and keep only one way to use them with from_variable
?
Wasn't there an idea to drop
secrets
section from step?
I am not aware of it. If it is just an idea without a proper discussion, maybe let's not act on that?
Maybe let's do not add variables section and keep only one way to use them with
from_variable
?
I have kept feature parity with secrets, to not cause confusion. If we want to remove it, wouldn't it be better to remove both secrets
and variables
together in a separate PR.
Just my thoughts, let me know if we still want to remove it.
If I set some variable and run pipeline
skip_clone: true
steps:
glob-vars:
image: alpine
commands:
- echo $$COMPANY
what would I see in steps (UI) logs?
Maybe add build_pr_images
label?
what would I see in steps (UI) logs?
It would be empty as you haven't explicitly added those variables in your step either by using variables
or from_variable
.
As this is not a secret value, it doesn't make sense to restrict this values by a step (to me). Like you mentioned
they will not be limited to specific events and plugins and will not be masked
So, I would expect it to be printed without explicitly added those variables in your step
.
In case if I want to map "global" var to different name in a step, I can use from_variable
.
@zc-devs
That is a good point. That would make this feature much easier to use.
Although, I feel that this kind of implicit behaviour might cause some confusion. It won't always be obvious where an environment variable is coming from.
Also, it could be confusing for a user who has push permissions on a repo, but cannot change the repo settings. This kind of user won't be able to view where the variables are automatically getting set from.
much easier to use
That is what I meant 👍
It won't always be obvious where an environment variable is coming from. user won't be able to view where the variables are automatically getting set from.
From Server or Agent 😄 I can say that about all those variables, that comes from Agent/Server :) (example)
I understand your point, but I'm still afraid of adding this new implicit behavior.
Can we get another opinion if that's okay?
Hi maintainers, it would be great to get a review from y'all.
I don't think setting them explicitly is a good idea as that could have unforeseen consequences that would be not so easy to track down both from usability and security perspective
Wasn't there an idea to drop
secrets
section from step?I am not aware of it. If it is just an idea without a proper discussion, maybe let's not act on that?
Maybe let's do not add variables section and keep only one way to use them with
from_variable
?I have kept feature parity with secrets, to not cause confusion. If we want to remove it, wouldn't it be better to remove both
secrets
andvariables
together in a separate PR.Just my thoughts, let me know if we still want to remove it.
Personally I don't see benefit of having multiple ways of using same feature, just adds confusion.
Also if we add it and later on remove we will have to again create more breaking changes that can be prevented without adding that in the first place
Also would be confusing if both variables and secrets would be used by the same name on what takes preference etc.
I only briefly skimmed through file changes but imho this introduces back security vulnerability with using variables
section with plugins
Thank you for the comments.
So from what I understood, I'll just keep one way to use variables, i.e., using from_variable
, and remove support for variables
in pipeline.
Then I'll complete the Cli and Docs changes for the final review.
@lafriks I have removed the support for using variables
, and completed the go client, cli and doc changes.
This PR is ready to be reviewed and merged.
Deploying preview to https://woodpecker-ci-woodpecker-pr-3723.surge.sh
Hi maintainers, I have finished working on this PR. This is ready for review.
before reading more into this pull code and issue, tow important questions:
pipeline/*
code at all? I thought we already allow to have environment viariablesAttention: Patch coverage is 5.63025%
with 1123 lines
in your changes missing coverage. Please review.
Project coverage is 26.02%. Comparing base (
3c8204a
) to head (856ff67
). Report is 1 commits behind head on main.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
What?
Why?