-
-
Notifications
You must be signed in to change notification settings - Fork 360
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
Implement repo level, user level and global environment variables support #3723
base: main
Are you sure you want to change the base?
Conversation
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: |
As variables are not secret anyways there was a suggestion somewhere to have a special config file like |
I'd prefer this way, but it's not that easy to have a good way to implement this at org/global level. |
JFYI, how it is implemented in Gitea. |
I think this is not as useful as it seems, since you can already use variables in a workflow file. This FYI, the solution I'm proposing is heavily inspired by Gitea. |
@anbraten @qwerty287 @zc-devs |
I can also add agent variables to address #3311 |
I like the idea.
I am not a right guy :) Let's hear from maintainers.
That would be great! |
I'm really interested in this PR. Keep moving! Thanks for your work. |
6954073
to
31082b2
Compare
I have completed the code changes in the server and web frontend. This means the feature is now in a fully usable state. It would be great if someone could review the code. Summary of Changes:
|
94119dd
to
380e9e5
Compare
Wasn't there an idea to drop |
I am not aware of it. If it is just an idea without a proper discussion, maybe let's not act on that?
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 Just my thoughts, let me know if we still want to remove it. |
It would be empty as you haven't explicitly added those variables in your step either by using |
As this is not a secret value, it doesn't make sense to restrict this values by a step (to me). Like you mentioned
So, I would expect it to be printed without |
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. |
That is what I meant 👍
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 |
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 |
Thank you for the comments. So from what I understood, I'll just keep one way to use variables, i.e., using Then I'll complete the Cli and Docs changes for the final review. |
138e534
to
8def7fd
Compare
@lafriks I have removed the support for using 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:
|
@@ -98,6 +98,16 @@ func (c *Compiler) createProcess(container *yaml_types.Container, stepType backe | |||
|
|||
workingDir = c.stepWorkingDir(container) | |||
|
|||
getVariableValue := func(name string) (string, error) { | |||
name = strings.ToLower(name) |
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.
we switched back to case sensitive for secrets ... so we should stay consistent
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 can still see name = strings.ToLower(name)
in the getSecretValue
even after merging main, should I still proceed to remove them from variables?
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.
let me have a look at the last pulls in this topic ...
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.
Any updates on this?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3723 +/- ##
==========================================
- Coverage 26.96% 26.02% -0.95%
==========================================
Files 394 409 +15
Lines 27416 28587 +1171
==========================================
+ Hits 7393 7439 +46
- Misses 19323 20437 +1114
- Partials 700 711 +11 ☔ View full report in Codecov by Sentry. |
Co-authored-by: 6543 <6543@obermui.de>
Co-authored-by: 6543 <6543@obermui.de>
What?
Why?