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

feat: adds a complete set of toolbox tasks #6

Merged
merged 3 commits into from
Oct 3, 2023
Merged

Conversation

Gowiem
Copy link
Member

@Gowiem Gowiem commented Oct 2, 2023

Info

  • Adds a completed set of working toolbox tasks (it was in a half implemented state before) for usage with clients.
    • This specifically adds working versions of the following:
      • run:pull
      • run:build
      • publish
      • clean
  • Adds a defined set of vars for local definition in a consumer project's .env.taskit

See the loom I shared in Slack for more details.

@Gowiem Gowiem self-assigned this Oct 2, 2023
Comment on lines 25 to 32
install:
desc: Install wrapper script from geodesic container
internal: true
requires:
vars:
- TOOLBOX_NAME
cmds:
- docker run --rm {{.REGISTRY_LATEST_URL}} init | bash -s {{.TOOLBOX_NAME}}
Copy link

Choose a reason for hiding this comment

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

it's impossible to understand what this task does from the code here. Please add a longer description, or if you can, pull the install wrapper script outside of the container to somewhere that it can be viewed

Copy link
Member Author

Choose a reason for hiding this comment

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

Great point -- I'll make this clearer via desc + summary. Thanks for the callout.

Comment on lines 67 to 84
run:
desc: Run our toolbox image while also mounting your $HOME folder to `/localhost` in the container
deps:
- install
cmds:
- docker run --rm {{.LATEST_IMAGE_TAG}} init | bash -s {{.IMAGE_NAME}} || (echo 'Try "sudo make install"'; exit 1)
- "{{.TOOLBOX_NAME}}"

pull:
desc: Pull our toolbox image from registry
deps:
- auth
requires:
vars:
- REGISTRY_URL_BASE
- REGISTRY_URL_FULL
- REGISTRY_LATEST_URL
cmds:
- |
aws ecr get-login-password --region $(AWS_REGION) | docker login --username AWS --password-stdin {{.REGISTRY_URL_BASE}}
docker pull {{.REGISTRY_URL_FULL}}
- docker pull {{.REGISTRY_LATEST_URL}}
- docker tag {{.REGISTRY_LATEST_URL}} {{.LATEST_IMAGE_TAG}}
Copy link

Choose a reason for hiding this comment

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

move these higher than run:pull and run:build so they can be read chronologically

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call -- will do 👍

internal: true
requires:
vars:
- TOOLBOX_NAME
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- TOOLBOX_NAME
- TOOLBOX_NAME
- REGISTRY_LATEST_URL


pull:
desc: Pull our toolbox image from registry
deps:
- auth
requires:
vars:
- REGISTRY_URL_BASE
Copy link
Member

Choose a reason for hiding this comment

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

Why is it required?

Copy link
Member Author

Choose a reason for hiding this comment

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

@gberenice REGISTRY_URL_BASE does not have a default and without a value, the REGISTRY_* vars will be broken. I added requiring here to ensure that we're passing a value from .env.taskit. Maybe there is a better way to do this?

Copy link
Member

Choose a reason for hiding this comment

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

Hm, probably not. I don't see a way to add a check to the global vars section. LATEST_IMAGE_TAG has a default, so probably just REGISTRY_URL_BASE needs to be required. But I don't feel strong about this, and I get why we'd want to check them all.

- auth
requires:
vars:
- REGISTRY_URL_BASE
Copy link
Member

Choose a reason for hiding this comment

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

Same here, one of them can be required if I get this right.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's continue this conversation in the above thread 👍

Comment on lines 74 to 85
install:
desc: Execute the toolbox wrapper script from geodesic, installing a toolbox start script onto the local host
summary: |
This executes the `init | bash` geodesic command, which pipes a bash script to the host machine which installs a
helper script onto the host. The helper script is used to easily start / attach the toolbox image as a container
and properly pass all of the relevant flags to docker (i.e. exposes a port, mounts $HOME to /localhost, etc.)
requires:
vars:
- TOOLBOX_NAME
- REGISTRY_LATEST_URL
cmds:
- docker run --rm {{.REGISTRY_LATEST_URL}} init | bash -s {{.TOOLBOX_NAME}}
Copy link

Choose a reason for hiding this comment

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

better but ideally there could be a link to the script itself so we can review that before running it on our machines 😁

Copy link
Member Author

Choose a reason for hiding this comment

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

@kevcube good idea, done 👍

Copy link
Member

@gberenice gberenice left a comment

Choose a reason for hiding this comment

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

🚢

@Gowiem Gowiem merged commit 9c02c6c into main Oct 3, 2023
1 check passed
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.

3 participants