Skip to content

Added devcontainer based on up-rust devcontainer#77

Merged
sophokles73 merged 3 commits into
eclipse-uprotocol:mainfrom
Xerner:main
Aug 27, 2025
Merged

Added devcontainer based on up-rust devcontainer#77
sophokles73 merged 3 commits into
eclipse-uprotocol:mainfrom
Xerner:main

Conversation

@Xerner
Copy link
Copy Markdown
Contributor

@Xerner Xerner commented Aug 16, 2025

Change Summary

Added a Docker devcontainer and its configuration. The goal of this is to provide a unified developer environment for users who want to make use of Dockers environment unifying power, and also use common VS Code extensions. It is also a personal preference, because I switch between using 3 computers and managing environments becomes cumbersome

Here is a gist of the specific features of using this implementation of a devcontainer

  • The base implementation for this devcontainer was copied from the up-rust repos devcontainer (https://github.com/eclipse-uprotocol/up-rust/tree/main/.devcontainer). It uses Microsofts Rust devcontainer image
  • It includes Docker mounts for a users SSH configuration. This way SSH settings used to push to places like Github are re-used (I use SSH with everything)

Welcome Bash Message

This is only seen once when creating a new terminal

image

@Xerner Xerner marked this pull request as ready for review August 16, 2025 23:26
@Xerner Xerner force-pushed the main branch 3 times, most recently from b71e92b to 20e1571 Compare August 18, 2025 04:55
@Xerner
Copy link
Copy Markdown
Contributor Author

Xerner commented Aug 18, 2025

Some explanation to all the force pushing. I made small updates to the added devcontainer commit, and just squashed the small changes into the added devcontainer commit using git rebase

... I also accidentally pushed an unwanted commit for this PR (b71e92b) and rolled it back

Copy link
Copy Markdown
Contributor

@PLeVasseur PLeVasseur left a comment

Choose a reason for hiding this comment

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

Hey @Xerner -- thanks for adding this in. Could you take a look at the feedback I left?

Comment thread .devcontainer/Dockerfile Outdated
Comment thread .devcontainer/devcontainer.json
Comment thread src/common/verify_filter_criteria.rs Outdated
@Xerner
Copy link
Copy Markdown
Contributor Author

Xerner commented Aug 23, 2025

I addressed your comments. I rebased the PR changes and squashed it all into a single commit. Let me know if this is unwanted/causes extra work. I personally just like to keep atomic changes into one single commit 🙂

Copy link
Copy Markdown
Contributor

@PLeVasseur PLeVasseur left a comment

Choose a reason for hiding this comment

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

More copyright notice bits in the bash scripts

Comment thread .devcontainer/scripts/bashrc_addition.sh
@PLeVasseur
Copy link
Copy Markdown
Contributor

I addressed your comments.

Hey, thanks!

I rebased the PR changes and squashed it all into a single commit. Let me know if this is unwanted/causes extra work. I personally just like to keep atomic changes into one single commit 🙂

I getchu. However, in the future after you've asked for review please keep all subsequent commits made for fixes / changes based on feedback.

It makes it easier when reviewing code when repeated review => update cycles have been done. ☺️

Copy link
Copy Markdown
Contributor

@PLeVasseur PLeVasseur left a comment

Choose a reason for hiding this comment

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

Thanks @Xerner for adding this functionality!

@PLeVasseur PLeVasseur requested a review from sophokles73 August 23, 2025 20:00
@PLeVasseur
Copy link
Copy Markdown
Contributor

@sophokles73 -- could you also review and approve?

--

@sophokles73 -- On another note: why is this set to need two reviewers? One is probably good, right?

Is this something that you'd know how to fix? Is this an Otterdog configuration we'd need to make?

Copy link
Copy Markdown
Contributor

@sophokles73 sophokles73 left a comment

Choose a reason for hiding this comment

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

This is really great and looks like it was a lot of work to create.
If I am not mistaken, we try to use a similar approach for multiple repos that are being maintained actively. It would be really cool if we could make sure that improvements/changes to the devcontainer configuration can be shared across the repos. I guess what I want to ask is: is it possible to reuse some/many/all of the config in some way across the repos? Otherwise I am a little concerned that configuration (and UX) will diverge over time ...

Comment thread .devcontainer/Dockerfile Outdated
Comment thread .devcontainer/devcontainer.json Outdated
@Xerner
Copy link
Copy Markdown
Contributor Author

Xerner commented Aug 25, 2025

This is really great and looks like it was a lot of work to create. If I am not mistaken, we try to use a similar approach for multiple repos that are being maintained actively. It would be really cool if we could make sure that improvements/changes to the devcontainer configuration can be shared across the repos. I guess what I want to ask is: is it possible to reuse some/many/all of the config in some way across the repos? Otherwise I am a little concerned that configuration (and UX) will diverge over time ...

You are correct. One solution that I would implement is putting the .devcontainer contents into its own repo, and then relating other respective repos using submodules. I'm aware that you guys already do this with up-spec. One concern I have with this is what happens when repos that rely on the devcontainer submodule start to use different versions of rustc or cargo... buuut then you could just make another .devcontainer branch/repo to address that.

If you guys want to go in the direction of using submodules, then when/if you guys make a devcontainer repo I'll PR these changes into it. I will also update the corresponding repos that already have duplicate devcontainers to use the submodule instead. Which are

  • up-rust
  • up-transport-iceoryx2-rust

Copy link
Copy Markdown
Contributor

@sophokles73 sophokles73 left a comment

Choose a reason for hiding this comment

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

LGTM

@sophokles73 sophokles73 merged commit 4cf6ea6 into eclipse-uprotocol:main Aug 27, 2025
2 checks passed
@sophokles73 sophokles73 added the enhancement New feature or request label Aug 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants