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

Force agent labels to be set on pipelines as well #3483

Closed
wants to merge 6 commits into from
Closed

Conversation

xoxys
Copy link
Member

@xoxys xoxys commented Mar 14, 2024

Fixes: #2634

@xoxys xoxys requested a review from a team March 14, 2024 21:12
@qwerty287
Copy link
Contributor

Wouldn't this be a breaking change?

@qwerty287 qwerty287 added agent enhancement improve existing features labels Mar 15, 2024
@xoxys
Copy link
Member Author

xoxys commented Mar 15, 2024

Yes it would. We can still decide to introduce another config option but as said in the issue the current behavior doesnt make sense to me.

@qwerty287
Copy link
Contributor

Yes, I agree to you, but then I think we should either make it non-breaking or wait for next major. Maybe we can keep the current behavior and log a deprecation warning if a label is missing?

@xoxys
Copy link
Member Author

xoxys commented Mar 15, 2024

What's the best way to make it none-breaking?

What I tried is to add a flag WOODPECKER_FILTER_LABELS_REQUIRED=true/false but how to pass it to the grpc.Next method? I think the attribute can be added to the rpc.Filter struct, but either way this would require changes to protobuf or am I wrong?

@xoxys xoxys marked this pull request as draft March 15, 2024 08:32
@qwerty287
Copy link
Contributor

Adding a new config var is one option, you could also just add a deprecation for now and change it in next major.
Can't help you with the grpc part though...

@xoxys
Copy link
Member Author

xoxys commented Mar 15, 2024

I thought about it again and to might be helpful for some use cases to keep the current behavior but make it configurable. @anbraten can you guide me a bit with the gRPC part?

@anbraten
Copy link
Member

What I tried is to add a flag WOODPECKER_FILTER_LABELS_REQUIRED=true/false but how to pass it to the grpc.Next method? I think the attribute can be added to the rpc.Filter struct, but either way this would require changes to protobuf or am I wrong?

To simplify it we could even make this a server setting (so no adjustments to grpc etc are needed) and change it to the new way with the next major.

As the agent sets some labels by default, hostname: * needs to be added as default to all tasks.

@qwerty287 qwerty287 added the breaking will break existing installations if no manual action happens label Jun 16, 2024
@qwerty287 qwerty287 added this to the 3.0.0 milestone Jul 17, 2024
@qwerty287 qwerty287 mentioned this pull request Jul 24, 2024
@qwerty287
Copy link
Contributor

@xoxys Now with 3.0 next, we could merge this I think?

@xoxys
Copy link
Member Author

xoxys commented Aug 12, 2024

Sure we could, I still think this should be better configurable but open for other decisions.

@6543
Copy link
Member

6543 commented Sep 24, 2024

this is not easy as the labels are currently used to let agents "advertice capabilitys/propertys" the pipelines can target.

so make them mandatory by default will break all existing pipelines and add unnecessary complexytiy to the config.

to make an label required should be optional!

I think we could either add some prefix like ! to indicate such .... etc

@6543
Copy link
Member

6543 commented Sep 24, 2024

image

ontop of #4141:

 ### `WOODPECKER_AGENT_LABELS`
 
 > Default: empty
 
 Configures custom labels for the agent, to let workflows filter by it.
 Use a list of key-value pairs like `key=value,second-key=*`. `*` can be used as a wildcard.
+If you use `!` as key prefix it is mandatory for the workflow to have that label set (without !) set and matched.
 By default, agents provide three additional labels `platform=os/arch`, `hostname=my-agent` and `repo=*` which can be overwritten if needed.
 To learn how labels work, check out the [pipeline syntax page](../20-usage/20-workflow-syntax.md#labels).

@xoxys xoxys closed this Nov 25, 2024
@xoxys xoxys deleted the fix-agent-filter branch November 25, 2024 08:30
@pat-s pat-s removed this from the 3.0.0 milestone Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent breaking will break existing installations if no manual action happens enhancement improve existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature to let agents require labels for pipelines
5 participants