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

Update dev container content #5906

Merged
merged 25 commits into from
Jan 17, 2023
Merged

Update dev container content #5906

merged 25 commits into from
Jan 17, 2023

Conversation

bamurtaugh
Copy link
Member

Based on team discussions and customer feedback, we've been assembling a list of docs to update: #5737.

Summary of changes:

  • Revisit Advanced Containers, removing one topic and adding a new one (help reduce the length of containers.md by extracting the git credential section into a new advanced page)
  • Update based on latest spec and CLI updates, terminology, and guidance
    • Major one was updating devcontainer.json examples to follow the latest spec and point to the latest images
  • Changes based on customer feedback, including content that was outdated or wording/info that was confusing
  • Separately, I updated aka.ms links along with CLI output info in containers.dev and devcontainers/cli
  • As the last remaining item in issue 5737, I'll also separately update our dev container Learn module after this

@bamurtaugh bamurtaugh requested review from Chuxel and chrmarti January 4, 2023 22:59
@bamurtaugh bamurtaugh self-assigned this Jan 4, 2023
@vscodenpa vscodenpa added this to the January 2023 milestone Jan 4, 2023
Copy link
Member

@samruddhikhandale samruddhikhandale left a comment

Choose a reason for hiding this comment

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

Looks great, left minor comments!

docs/devcontainers/create-dev-container.md Outdated Show resolved Hide resolved
docs/devcontainers/tutorial.md Outdated Show resolved Hide resolved
docs/remote/ssh.md Outdated Show resolved Hide resolved
remote/advancedcontainers/start-processes.md Outdated Show resolved Hide resolved
bamurtaugh and others added 4 commits January 4, 2023 15:11
Co-authored-by: Samruddhi Khandale <skhandale@microsoft.com>
Co-authored-by: Samruddhi Khandale <skhandale@microsoft.com>
Co-authored-by: Samruddhi Khandale <skhandale@microsoft.com>
Co-authored-by: Samruddhi Khandale <skhandale@microsoft.com>
@bamurtaugh
Copy link
Member Author

Thanks so much for the fast review, @samruddhikhandale! Merged all your suggestions.

Copy link
Member

@samruddhikhandale samruddhikhandale left a comment

Choose a reason for hiding this comment

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

Thanks! Missed -bullseye in two other places.

docs/devcontainers/tutorial.md Outdated Show resolved Hide resolved
docs/devcontainers/tutorial.md Outdated Show resolved Hide resolved
bamurtaugh and others added 2 commits January 4, 2023 15:55
Co-authored-by: Samruddhi Khandale <skhandale@microsoft.com>
Co-authored-by: Samruddhi Khandale <skhandale@microsoft.com>
@bamurtaugh
Copy link
Member Author

Thanks! Just merged those changes in.

@Chuxel
Copy link
Member

Chuxel commented Jan 5, 2023

In the CLI section, we still tell people to add their features again to the secondary devcontainer.json. That's no longer required in the image pre-build situation so...

  1. Create a simplified devcontainer.json file in repositories where you'd like to use the image - the devcontainer.json should either use the image property or reference the image in an associated Docker Compose file. For example:

    {
        "image": "ghcr.io/your-org/your-image-name"
    }

@bamurtaugh
Copy link
Member Author

@Chuxel thanks so much for the detailed review! I believe I've addressed and replied to your comments above.

@Chuxel
Copy link
Member

Chuxel commented Jan 5, 2023

One thing I'm not sure we have a good link to anywhere is image metadata. We allude to it in the pre-builds section but we're a bit more specific in the containers.dev docs.

https://containers.dev/implementors/reference/#labels

EDIT: In fact, I noticed a couple bad links, and refreshed the Dev Container spec section on this as well: devcontainers/devcontainers.github.io#122

@bamurtaugh
Copy link
Member Author

Thanks @Chuxel! I expanded the info on metadata in the pre-build section (like what you added in the linked containers.dev PR), and I added links to this content from the Notes I had added on metadata throughout other docs.

Co-authored-by: Chuck Lantz <chuck_lantz@hotmail.com>
@Chuxel Chuxel self-requested a review January 7, 2023 01:30
Chuxel
Chuxel previously approved these changes Jan 7, 2023
@bamurtaugh
Copy link
Member Author

@gregvanl This is ready for your review and merge whenever you're ready!

chrmarti
chrmarti previously approved these changes Jan 9, 2023
Copy link
Contributor

@chrmarti chrmarti left a comment

Choose a reason for hiding this comment

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

LGTM, 2 nits.

docs/devcontainers/containers.md Outdated Show resolved Hide resolved
remote/advancedcontainers/sharing-git-credentials.md Outdated Show resolved Hide resolved
@bamurtaugh bamurtaugh dismissed stale reviews from chrmarti and Chuxel via 47eaf31 January 9, 2023 17:09
@bamurtaugh bamurtaugh requested a review from gregvanl January 13, 2023 20:58
@gregvanl gregvanl merged commit c9afa4e into main Jan 17, 2023
@gregvanl gregvanl deleted the bamurtaugh/containers-edits branch January 17, 2023 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants