Skip to content

Conversation

@scottrigby
Copy link
Contributor

@scottrigby scottrigby commented May 2, 2025

Collaboration during today's meeting with @hedge-sparrow, @banjoh and @0xJMart

Update: made some larger cleanups.

This PR:

  • added CMD var to dev:shell
    • (also moved this task to top of taskfiles/container.yaml to make it more obvious this is our entrypoint for all other commands. Could move that back to where it was for easier diff if we prefer)
  • moved our Taskfile to /internal
  • mounts internal Taskflie over top of root taskfile only in container
  • result is the exact same command names that just call the long-form internal commands only within the container

@scottrigby scottrigby force-pushed the in-container-only branch from ffbe8ca to e45d379 Compare May 2, 2025 15:54
@scottrigby scottrigby marked this pull request as ready for review May 2, 2025 15:57
Copy link
Member

@hedge-sparrow hedge-sparrow left a comment

Choose a reason for hiding this comment

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

A few notes from me

@nvanthao nvanthao mentioned this pull request May 7, 2025
2 tasks
Copy link
Member

@hedge-sparrow hedge-sparrow left a comment

Choose a reason for hiding this comment

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

lgtm

@hedge-sparrow
Copy link
Member

Since there's a few PRs waiting on the guts of this to land, I suggest we merge it asap and open a second PR to address any documentation concerns.

Copy link
Member

@xavpaice xavpaice left a comment

Choose a reason for hiding this comment

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

I haven't reviewed this in depth, but note that there are some review comments on #59 which relate to commits in this PR. These need addressing prior to merge.

# Install yq
&& BINARY=yq_linux_amd64 \
&& VERSION=v4.45.1 \
&& curl -Ls https://github.com/mikefarah/yq/releases/download/${VERSION}/${BINARY}.tar.gz -O \
Copy link
Member

Choose a reason for hiding this comment

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

from #59, Chris asks "Why did you move yq out of the package manager and into manual install? Are we using something that's so new it's not in packages yet?"

Copy link
Member

Choose a reason for hiding this comment

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

I believe this is because the packaged yq in ubuntu is ancient.

- '{{.CONTAINER_RUNTIME}} ps | grep -q "{{.DEV_CONTAINER_NAME}}"'
vars: [CONTAINER_NAME, IMAGE_NAME]
deps:
- task: check-image-exists
Copy link
Member

Choose a reason for hiding this comment

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

See Chris' comment #59 (comment)

"The security implications of this are a bit concerning, although if it's necessary it's not something I would loose sleep over.

Can you explain why kubectl port-forward is needed? Is this running port-forward from within the container, or external to the container? Is there a reason you can't just expose a port rather than use host networking?"

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I entirely understand what security implications we're referencing here? we're running a container that we build ourselves with a few tools that we would have otherwise been running on our hosts without any sandboxing.

The primary vector for compromise here might be a MITM attack against the registry we're pushing an image to, but I'm not entirely sure on the utility of storing the dev workflow image in an online registry to begin with, my preference would have been to keep the build in the local image cache.

Copy link
Member

Choose a reason for hiding this comment

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

I just mean that using host networking bypasses the containerization, however I think this comment is kind of moot after this mornings standup if we're moving to just run on the host anyway and leaving containers as an advanced topic.

@scottrigby
Copy link
Contributor Author

In a recent CRE sync the team dislikes all of the working syntax options we found so far to make taskfile play nicely with running tasks with args inside containers. They add extra complexity and require explanation to users who want to modify or add tasks. I also proposed the idea of making a simple example repo with only deployment patterns, and i'll do that when i get a little time--once i do, something like this could be added there for discussion.

@scottrigby scottrigby closed this May 16, 2025
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.

4 participants