Skip to content
This repository has been archived by the owner on Nov 30, 2023. It is now read-only.

Salesforce/sfdx project container #123

Merged
merged 2 commits into from
Aug 28, 2019

Conversation

dehru
Copy link
Contributor

@dehru dehru commented Aug 27, 2019

This adds a dev container definition for sfdx based on the dockerhub container salesforce/salesforcedx with additional extensions defined in devcontainer.json. I didn't include a test-project directory because we have separate test coverage for salesforce/salesforcedx.

@msftclas
Copy link

msftclas commented Aug 27, 2019

CLA assistant check
All CLA requirements met.

@Chuxel
Copy link
Member

Chuxel commented Aug 28, 2019

Thanks so much for the contribution! Looks good (though I don't have a project to test in depth), but two thoughts for consideration:

  1. If you are targeting developers running Linux locally, one thing to be aware of is that, when you are running in the container as root, files or folders created in the container are created as root on the local side as well. (This doesn't happen in Windows or macOS). We've updated most of the containers in this repo include a non-root user for that reason in their Dockerfiles and a commented out "runArgs" property in devcontainer.json. More info here. The basic ubunutu-18.04-git definition is an example.

  2. If you just want people to use the image unmodified, you can also drop the Dockerfile entirely and use the image property instead of dockerFile. That said, having the Dockerfile does make it easier to add additional dependencies, so it may still make sense to keep it if you want to.

Either of these make sense in this case?

@vazexqi
Copy link

vazexqi commented Aug 28, 2019

@Chuxel - Thanks for the feedback.

  1. Yes using a non-root user is a good idea. For this, we will need to do a bit more testing. The majority of our users (>90%) are on Windows and Mac so this won't be a blocker. If we need to, we will add the non-root user directly to the salesforce/salesforcedx Dockerfile.
  2. Yes, we intentionally used the Dockerfile approach instead of just referencing it for the reasons you mentioned. It is easier for a user to customize it.

So the current state is good for us and we will iterate by making changes through our Dockerfile at (soon to be public) https://github.com/forcedotcom/salesforcedx-docker

With that, are we good to merge?

@dehru
Copy link
Contributor Author

dehru commented Aug 28, 2019

Once this is merged, when does it become available in the VS Code?

@Chuxel
Copy link
Member

Chuxel commented Aug 28, 2019

@vazexqi Yep! Cool stuff!

@dehru We're in our "endgame" for August (VS Code 1.38), so this should land in insiders soon and go out with the stable release sometime next week. We can push out-of-band if needed, but tend to stay on the same release cadence since VS Code releases tend to be when people notice updates.

@Chuxel Chuxel merged commit 94f940d into microsoft:master Aug 28, 2019
@dehru dehru deleted the salesforce/sfdx-project-container branch August 28, 2019 19:29
@Chuxel
Copy link
Member

Chuxel commented Aug 28, 2019

@vazexqi @dehru One thing we happend to notice in the doc update you referenced, was:

">NOTE: Make sure you clear salesforcedx-vscode-apex.java.home from your local settings because it overrides the default setting in the container environment."

Just a FYI - but are you aware of the "settings" property in devcontainer.json?

You can use it like this to set a container specific setting that takes precedence over the user setting.

"settings": {
    "salesforcedx-vscode-apex.java.home": "/usr/local/jvm/java-11-openjdk-amd64"
}

Adding it to devcontainer.json automatically sets it when the container is created (or rebuilt). You can also edit it from the settings UI once you're connected to the container (under a "Remote" tab), but this automates it. Dev containers like Java explicitly set this for exactly this reason.

Not sure if the path above is right, but this could easily get added in.

@vazexqi
Copy link

vazexqi commented Aug 28, 2019

Awesome, thanks for tip @Chuxel ! We will do that.

@Chuxel
Copy link
Member

Chuxel commented Aug 28, 2019

@vazexqi Great - Happy to review a PR (or just make the change if easier).

@dehru
Copy link
Contributor Author

dehru commented Aug 28, 2019

Thanks for the tip. I just submitted a new PR with this property set.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants