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

build: Add devcontainer #117

Merged
merged 22 commits into from
Jan 21, 2024

Conversation

mbtamuli
Copy link
Contributor

This is a draft PR in relation to #96.

@mbtamuli
Copy link
Contributor Author

I'm working on adding documentation.

@mugdha-adhav @vadasambar @gesarki If any of you are familiar with/use VSCode, could you help with early testing for this and providing feedback, if any?

Would be good to know if people would benefit from this, especially considering the differences of - amd64 vs arm64 and docker builds with Apple Silicon etc.

Please consider this "alpha" state feature. I'm actively iterating on this. 😅

You should be able to

  • checkout the branch
    # If you have the GitHub CLI
    gh pr checkout 117
    code .
    
  • open the directory in VSCode
  • click on Reopen in Container
    image
  • Alternatively, you can run the command Dev Containers: Reopen in Container (Default VSCode key combination: Shift + Command + P (Mac) / Ctrl + Shift + P (Windows/Linux))

@gesarki
Copy link
Contributor

gesarki commented Jan 15, 2024

I haven't used devcontainers before, but I followed your instructions and looks like it's working:
image

Small caveat - the first time it failed with an error about the SHAs not matching. After re-running it works as it should I think.
Good start!

@mbtamuli mbtamuli force-pushed the build/add-devcontainer branch 3 times, most recently from a175043 to d923e35 Compare January 17, 2024 06:26
@mbtamuli
Copy link
Contributor Author

I've tested this with devcontainer cli, CodeSpaces and VSCode.

@mugdha-adhav There's a pre-requisite to this dev container, we'll have to pre-build the dev container image.

Now ideally we should have a separate repository for the pre-build files. Then we'd have the files for using dev container as in this PR. Currently I have a branch in my fork - mbtamuli#2. That is where I'm building the image ghcr.io/mbtamuli/csi-driver-image:latest currently being used in the devcontainer.json in this PR.

We can still keep the pre-build files in the repository, but the repository structure could become confusing.

.
├── .devcontainer
│   ├── devcontainer.json
│   └── welcome.txt
├── .github
│   ├── .devcontainer
│   │   └── devcontainer.json
│   └── workflows
│       ├── devcontainer-build-and-push.yaml

Let me know what you think.

@mugdha-adhav
Copy link
Collaborator

Now ideally we should have a separate repository for the pre-build files.

TBH, it doesn't make much sense to me to create a new repository just for maintaining these files.

We can still keep the pre-build files in the repository, but the repository structure could become confusing.

We could add more notes if needed to add more clarity. Also these files are hidden, so most of the times they would be ignored unless we need to fix the dev-containers themselves, in which case the documentation would come in handy.

@mbtamuli
Copy link
Contributor Author

📝 Got it.

Okay, I'll update this PR with all the necessary changes and get this PR ready for review.

@mbtamuli
Copy link
Contributor Author

@mugdha-adhav I'll need some help with this PR.

I'm trying to create the image - ghcr.io/warm-metal/csi-driver-image/devcontainer. GHCR allows creating/pushing the image automatically if the image name matches the repository name. If not, you can still push but you need to create the package manually and allow permissions to the repository.

You'll need to

  1. Login using these instructions - Authenticating with a personal access token (classic). If you have the GitHub CLI installed, things are much easier.
    export GITHUB_TOKEN="$(gh auth token)"
    echo $GITHUB_TOKEN | docker login ghcr.io -u <username> --password-stdin
    
  2. Push a docker image. You can even retag the alpine image and push.
    docker pull alpine
    docker tag alpine ghcr.io/warm-metal/csi-driver-image/devcontainer
    docker push ghcr.io/warm-metal/csi-driver-image/devcontainer
    
  3. Follow the instructions here - Ensuring workflow access to your package

@mbtamuli
Copy link
Contributor Author

Also, is there any reason to stick to Go version 1.19? If not, I'll take the opportunity to update any references to Go version 1.21

@mugdha-adhav
Copy link
Collaborator

Also, is there any reason to stick to Go version 1.19? If not, I'll take the opportunity to update any references to Go version 1.21

I don't see a reason to stop/pause the upgrade. Just need to ensure that the go dependencies are compatible with the newer version.

Also the current node-image that we are building and using in the CI for containerD and crio is for k8s v1.25. Hence the dependencies were pinned to v1.25. We should have a separate milestone for supporting and deprecating k8s versions.

@mbtamuli
Copy link
Contributor Author

I don't see a reason to stop/pause the upgrade. Just need to ensure that the go dependencies are compatible with the newer version.

Sure. Will keep in mind and add the upgrades.

Also the current node-image that we are building and using in the CI for containerD and crio is for k8s v1.25. Hence the dependencies were pinned to v1.25. We should have a separate milestone for supporting and deprecating k8s versions.

Not tackling k8s upgrades.

I see you've reacted to my earlier comment - #117 (comment), please ping me once it's done.

@mugdha-adhav
Copy link
Collaborator

please ping me once it's done

@mbtamuli I have created the package and provided GitHub actions with the admin access to the package. Could you please verify if it's working?

Also appreciate the detailed write-up. JFYI, we needed to create a PAT with the required scope for packages (default one only is scoped to 'gist', 'read:org', 'repo').

@mbtamuli
Copy link
Contributor Author

@mugdha-adhav No, it doesn't seem to be working. Also, I don't see the package. Maybe you need to make it public?

https://github.com/orgs/warm-metal/packages?repo_name=csi-driver-image

Signed-off-by: Mriyam Tamuli <[email protected]>
Signed-off-by: Mriyam Tamuli <[email protected]>
Signed-off-by: Mriyam Tamuli <[email protected]>
Signed-off-by: Mriyam Tamuli <[email protected]>
Signed-off-by: Mriyam Tamuli <[email protected]>
Signed-off-by: Mriyam Tamuli <[email protected]>
Signed-off-by: Mriyam Tamuli <[email protected]>
Signed-off-by: Mriyam Tamuli <[email protected]>
@mbtamuli
Copy link
Contributor Author

JFYI, we needed to create a PAT with the required scope for packages (default one only is scoped to 'gist', 'read:org', 'repo').

What do you mean? Did you create a PAT for running docker login locally?

@mugdha-adhav
Copy link
Collaborator

JFYI, we needed to create a PAT with the required scope for packages (default one only is scoped to 'gist', 'read:org', 'repo').

What do you mean? Did you create a PAT for running docker login locally?

Yes, I did it by creating a token. And verified that we could also do it using gh auth login -s write:packages.

Signed-off-by: Mriyam Tamuli <[email protected]>
Signed-off-by: Mriyam Tamuli <[email protected]>
Signed-off-by: Mriyam Tamuli <[email protected]>
Signed-off-by: Mriyam Tamuli <[email protected]>
Signed-off-by: Mriyam Tamuli <[email protected]>
Signed-off-by: Mriyam Tamuli <[email protected]>
Signed-off-by: Mriyam Tamuli <[email protected]>
Signed-off-by: Mriyam Tamuli <[email protected]>
Signed-off-by: Mriyam Tamuli <[email protected]>
@mbtamuli
Copy link
Contributor Author

mbtamuli commented Jan 21, 2024

@mugdha-adhav Seems like a workflow run on PR from a fork won't have the right access to publish a package for the Workflow. Please check the run from my repository. I created a PR against my own repo to simulate the action.

Let's try merging this action. I believe it should work. If not, I can raise a follow up PR if it needs fixing.

Related reading: https://github.com/orgs/community/discussions/69331

@mbtamuli mbtamuli marked this pull request as ready for review January 21, 2024 16:30
@mbtamuli mbtamuli requested a review from a team as a code owner January 21, 2024 16:30
@mugdha-adhav
Copy link
Collaborator

Thanks for updating the docs as well!

@mugdha-adhav mugdha-adhav merged commit 46df7e8 into warm-metal:master Jan 21, 2024
6 of 7 checks passed
@mbtamuli mbtamuli deleted the build/add-devcontainer branch January 22, 2024 04:26
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