Conversation
|
There are currently some comments in the install.sh file so I can debug CI checks if needed. I will remove them before final approval. |
.github/workflows/test.yaml
Outdated
| # Debian-based | ||
| - debian:latest | ||
| - debian:bullseye | ||
| - ubuntu:latest | ||
| - ubuntu:22.04 | ||
| - ubuntu:20.04 | ||
| # Microsoft dev containers | ||
| - mcr.microsoft.com/devcontainers/base:ubuntu | ||
| - mcr.microsoft.com/devcontainers/base:debian |
There was a problem hiding this comment.
optional: I think testing on just the latest images is probably fine.
src/doppler-cli/install.sh
Outdated
| echo "Checking for curl..." | ||
| if ! type curl >/dev/null 2>&1; then | ||
| echo "Installing curl..." | ||
| apt-get update -y && apt-get -y install --no-install-recommends curl ca-certificates |
There was a problem hiding this comment.
blocking: This all assumes that this is a debian based system with apt-get installed, which is not a valid assumption. Take a look at how some of microsoft's features install from different package managers. We should support redhat variants and alpine, at a minimum (and probably also test those).
There was a problem hiding this comment.
Also, be sure to clean up the package lists after you're done, to keep the user's layer size small.
.github/workflows/release.yaml
Outdated
| push: | ||
| branches: [main] |
There was a problem hiding this comment.
nit: I don't know that we necessarily want this on every push, we likely want it to be manual and only run it when the version changes.
There was a problem hiding this comment.
Good call, removed this.
| { | ||
| "source": "doppler-cli-user-config", | ||
| "target": "/doppler", | ||
| "type": "volume" | ||
| } |
There was a problem hiding this comment.
check: Is it safe/valid to have a volume shared between all containers that might use this feature?
There was a problem hiding this comment.
This is a good question and I struggled with where to go for this. The volume gets shared across devcontainers, but the devcontainers stay in 1 location. Meaning if someone has multiple devcontainers running, those containers should be contained to their local machines and not be shared with other engineers. Where this could become an issue is if a team is working on a shared Docker host, but that seems very atypical of devcontainer usage. Having a volume means users don't have to reauthenticate across every dev container if they're running multiple or on every single devcontainer build. I could be talked into either direction on this.
There was a problem hiding this comment.
It's been a long time since I looked at this, but as I recall, the volumes get a persistent, unique ID appended to the name when the container is started, so I don't think this should be a problem unless they're literally using the exact same devcontainer, which seems unlikely. Most ways I've seen this used, each developer has their own devcontainer running.
There was a problem hiding this comment.
I've pulled the volume. It's easy enough for any user to add a volume to their own devcontainer.json file which gives users the option of going either way.
03fc764 to
e314e70
Compare
f871b92 to
62cafcc
Compare
|
Notes since last reviews and opening PR:
|
emily-curry
left a comment
There was a problem hiding this comment.
The behavior itself seems solid! Just needs some final polish to actually make this public.
| packages: write | ||
| steps: | ||
| - uses: actions/checkout@v3 | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
nit: In the future, this is the category of change that should be in its own commit, not squashed into the same commit as the main feature.
There was a problem hiding this comment.
nit: We will want to add/update the README for both the feature itself, and in the root of the repository. This will be a public repo. Here's an example of how another open source project does it: https://github.com/rails/devcontainer/blob/main/features/src/mysql-client/README.md
We should also move the .devcontainer.json to be in .devcontainer/devcontainer.json to match the latest in the template repo, and change the license to be the apache license (which will then need to be linked to with the "licenseURL" property).
There was a problem hiding this comment.
It does look like the feature docs in src/doppler-cli/README.md are auto-generated, so those don't need to be touched.
There was a problem hiding this comment.
@emily-curry updated the license, README, and folder structure. I tried to follow other public repos, but open to any other feedback as always.
emily-curry
left a comment
There was a problem hiding this comment.
Looks good! After you merge, I'll move this to the main DopplerHQ org and cut a release.
This PR adds a few things to the POC that already existsed:
This will require CLI changes to be deployed in order for CI tests to pass: PR 512