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

Add labels to run container #376

Merged

Conversation

francoiscampbell
Copy link
Contributor

Adds labels that can be useful for finding the containers spawned on a host by this plugin. This can also enable automated tools to associate these docker-compose containers with the agent that started them.

Tested on our infra:
image

Question for maintainers: should this be behind a plugin config? I don't think so since it doesn't affect the behaviour of the container itself, but I'm happy to make it conditional if needed.

@francoiscampbell francoiscampbell marked this pull request as draft February 27, 2023 21:55
@toote
Copy link
Contributor

toote commented Mar 1, 2023

This is a great addition! 👏

I would indeed add it behind a configuration, but have it on by default... the reason is two-fold:
1- whoever doesn't want/need the behaviour will be able to turn it off
2- it will make it simpler to not have to change all 75 tests and only turn it on in the feature-specific tests 😉 😉

@francoiscampbell francoiscampbell marked this pull request as ready for review March 3, 2023 20:45
@francoiscampbell
Copy link
Contributor Author

Thanks for the feedback @toote, made it conditional (default true), updated the existing tests to disable the option, and added a new test.

Copy link
Contributor

@toote toote left a comment

Choose a reason for hiding this comment

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

👏 @francoiscampbell

Just a minor comment and a cosmetic suggestion.

If you feel like making the changes, I would also suggest updating the plugin's versions in the readme with the next one (v4.11.0) to avoid having to create another PR in the future. But there is no hurry or need and we might merge and release this plugin without that change

tests/push.bats Outdated Show resolved Hide resolved
tests/run.bats Outdated Show resolved Hide resolved
Copy link
Contributor

@pzeballos pzeballos left a comment

Choose a reason for hiding this comment

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

Nice!

@pzeballos pzeballos merged commit 14ae3ba into buildkite-plugins:master Mar 9, 2023
@francoiscampbell francoiscampbell deleted the pipeline-labels branch July 5, 2023 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants