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

Provide ability to run a pre bundle update script #7

Merged
merged 33 commits into from Aug 9, 2019

Conversation

@joesustaric
Copy link
Contributor

commented Aug 5, 2019

Context

I was attempting to use this plugin, and came across an issue where I needed to install a app which was needed for the bundle update to pass.
The application that I'm configuring does not have it's 'own' docker container nor do I want to create and manage one seeing as thought the bundle update process will only be executed once a week or so. This means to me that build times are irrelevant and I don't mind the build taking a long time building a container.

This PR is the result of my 2nd approach, which seems a lot simpler than my first approach.
This PR just uses a simple convention of 'put the script here and if it exists, it will run' similar to the hooks. I chose .buildkite/scripts/pre-bundle-update for this. hooks appears to be used more for the buildkite functionality so a scripts directory seemed more appropriate.

I've tested this very simply and it appears to work ok.

FYI the second approach was with a defined parameter in the pipeline.yml to point to the script directory. My noobness with writing buildkite plugins seem to get the better of me and I found it difficult to get it working correctly. (as you can see with the commit history)
So I defaulted back to the convention approach with also seems a lot simpler in implementation.

UPDATE

gone back to the script being passed in explicitly. Seems just as easy to just pass it into the docker command.

joesustaric added some commits Aug 2, 2019

@joesustaric joesustaric requested a review from orien Aug 5, 2019

joesustaric added some commits Aug 5, 2019

@orien

This comment has been minimized.

Copy link
Member

commented on f2a44cc Aug 6, 2019

FYI: There's a reference repo here https://github.com/buildkite/emojis

joesustaric added some commits Aug 6, 2019

@orien

orien approved these changes Aug 8, 2019

Copy link
Member

left a comment

Fantastic work @joesustaric! This is a valuable contribution. Thanks.

plugins:
- envato/bundle-update#v0.7.0:
update: true
pre-bundle-update: .buildkite/scripts/pre-bundle-update

This comment has been minimized.

Copy link
@orien

orien Aug 8, 2019

Member

We should add this parameter to the configuration section of the readme too.

@@ -2,4 +2,12 @@
set -euo pipefail

cd /bundle_update

if [ -f "$PRE_BUNDLE_UPDATE" ]; then

This comment has been minimized.

Copy link
@orien

orien Aug 8, 2019

Member

Perhaps we don't need this conditional. If the script doesn't exist, I'm thinking it makes sense that the bundle update should fail.

This comment has been minimized.

Copy link
@orien

orien Aug 8, 2019

Member

Additionally, as we're using eval on line 8, it doesn't have to be a script. A straight up command could be provided:

steps:
  - label: ":bundler: Update"
    plugins:
      - envato/bundle-update#v0.7.0:
          update: true
          pre-bundle-update: "apk add --no-progress build-base"

This comment has been minimized.

Copy link
@joesustaric

joesustaric Aug 8, 2019

Author Contributor

Interesting. Yep you are right it should fail if you have spedified a script and it does not exist. I will update that.

In regards to providing commands via the yml.. I'm not a fan of that tbh.
It feels nicer if a script holds all these commands, easier to locate run, debug, etc..
I might remove the eval and just do a .

This comment has been minimized.

Copy link
@orien

orien Aug 8, 2019

Member

It does provide options to others who may think differently though:

A whole extra file to store one command

And it's consistent with the top level Buildkite step command attribute.

This comment has been minimized.

Copy link
@joesustaric

joesustaric Aug 9, 2019

Author Contributor

humm ok fair enough.. I'll trust that developers wont write a 300 line shell script as a yaml parameter :D

joesustaric added some commits Aug 9, 2019

run whatever is passed in script or command, it should exit and fail …
…if the command or script return a non zero exit code
@orien

orien approved these changes Aug 9, 2019

Copy link
Member

left a comment

Thanks. This is awesome!

@joesustaric joesustaric merged commit a1e4a67 into master Aug 9, 2019

5 checks passed

buildkite/bundle-update-buildkite-plugin Build #127 passed (39 seconds)
Details
buildkite/bundle-update-buildkite-plugin/hammer-test Passed (20 seconds)
Details
buildkite/bundle-update-buildkite-plugin/pipeline Passed (9 seconds)
Details
buildkite/bundle-update-buildkite-plugin/shell-shellcheck Passed (11 seconds)
Details
buildkite/bundle-update-buildkite-plugin/sparkles-lint Passed (13 seconds)
Details

@joesustaric joesustaric deleted the pre-bundle-update-hook branch Aug 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.