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

First stab at new devcontainer #233

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Conversation

robarnold
Copy link
Contributor

No description provided.

@robarnold robarnold marked this pull request as ready for review December 29, 2024 22:38
@robarnold robarnold requested a review from sdwilsh December 29, 2024 22:39
.devcontainer/post-create-command.sh Outdated Show resolved Hide resolved
.devcontainer/devcontainer.json Outdated Show resolved Hide resolved
.devcontainer/devcontainer.json Show resolved Hide resolved
.devcontainer/Dockerfile Outdated Show resolved Hide resolved
.github/workflows/checks.yml Show resolved Hide resolved
[group('test')]
test: kustomization-tests

regen-yarn-sdks:
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah...this is how it is generated. It might be nice to add a comment to the files after generated in this to say it's generated and how to rerun.

Do we need to add CI to make sure we don't need to apply changed to this with updates?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm wary of touching the files - it's probably safer to leave this as a black box, but I don't know when it should be updated

Copy link
Contributor

Choose a reason for hiding this comment

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

I figure we can make a job that runs this, and if there's any code changes, it'll fail and we know we need to handle that. Alternatively...we could just run this as a setup to our devcontainer and then add it to the gitignore file so we don't even version it.

Neither is blocking for this PR.

justfile Show resolved Hide resolved
justfile Outdated
hado-lint:
#!/usr/bin/env bash
set -euo pipefail
find . -name "Dockerfile*" -print | xargs -r -n1 hadolint
Copy link
Contributor

Choose a reason for hiding this comment

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

kustomization-tests:
#!/usr/bin/env bash
set -euo pipefail
find kustomization/tests -mindepth 1 -maxdepth 1 -type d -print | \
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably a good idea to do a loop here as well.

justfile Outdated Show resolved Hide resolved
"version": "2.12.0"
},
"ghcr.io/devcontainers/features/node:1": {
// TODO: figure out how to version node
Copy link
Contributor

@sdwilsh sdwilsh Jan 2, 2025

Choose a reason for hiding this comment

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

From the docs, it looks like we can just supply version here: https://github.com/devcontainers/features/tree/main/src/node

I think we want v23.5.0. I don't know if you care to also version npm, etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this and the other one, I wasn't sure what the renovate line should look like. Separately, the risk of picking a specific version here is that it gets pulled from upstream quickly and then your build breaks. Mostly I noticed this with the gh cli

Copy link
Contributor

Choose a reason for hiding this comment

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

We can always have renovate wait a few days to help I guess?

},
"features": {
"ghcr.io/devcontainers/features/docker-in-docker:2": {
// TODO: figure out docker versioning story here
Copy link
Contributor

@sdwilsh sdwilsh Jan 2, 2025

Choose a reason for hiding this comment

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

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.

2 participants