Skip to content

Conversation

@davidblum
Copy link
Contributor

Overview

This PR adds optional configuration for the base shellcheck image.

Rationale

Our organization prefers to store docker images in an internal registry, by enabling this configuration
option, we can continue to use the plugin with our specified registry

@davidblum davidblum requested a review from a team as a code owner September 30, 2024 17:45
@mcncl
Copy link
Contributor

mcncl commented Oct 1, 2024

@davidblum thanks for the PR! I've added some tests for your additional config, hope that's OK!

@davidblum
Copy link
Contributor Author

@mcncl Awesome!

Thank you very much for adding those!

It looks like the building is failing, I'll see if I can figure anything out.

@jeremybumsted
Copy link
Contributor

@davidblum looks like the build is failing because the selftest step is trying to check out the commit which only exists on your fork. Let me see if I can get it sorted so we can get a green build, but I think as long as the other tests pass I'm happy for us to merge this so we aren't blocking you.

@davidblum
Copy link
Contributor Author

@jeremybumsted ah, that makes sense.

I'm ok with that plan as long as it work for you and the team!

@davidblum
Copy link
Contributor Author

@jeremybumsted I wasn't able to make any progress on my end.

Have you made any progress?

@jeremybumsted
Copy link
Contributor

@davidblum I think I've got a solution, see this commit.

And looks like we got it: https://buildkite.com/buildkite/plugins-shellcheck/builds/62#01924e97-b137-4009-927e-a6dba0f24fd5/3-4

I also added another test just to try and cover all our bases on this change 😄

Copy link
Contributor

@jeremybumsted jeremybumsted left a comment

Choose a reason for hiding this comment

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

This looks good - both @mcncl and myself have had a look through this and added some tests to cover some scenarios for setting that custom image - thanks for raising this pr @davidblum 🙌

@jeremybumsted jeremybumsted merged commit 415387e into buildkite-plugins:master Oct 2, 2024
@davidblum
Copy link
Contributor Author

Woohoo!

Thank you both so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants