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

redundant dockerfile issue with build #812

Conversation

prathameshzarkar9
Copy link
Contributor

#791 Redundant Dockerfile required in order to push to correct repo

if --image-name is provided in the cli then it should not push to cli-generated-image. The cli should push to user provided repository.

if we pass this argument value then it is stored in the additionaImageNames parameter which is used to check the additional names are provided. if yes, then it should not use cli-generated-image accordingly modification have been incorporated.

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.

Overall the change makes sense to me, left some comments. Thanks!

.vscode/launch.json Outdated Show resolved Hide resolved
src/test/configs/example-push/.devcontainer.json Outdated Show resolved Hide resolved
src/spec-node/containerFeatures.ts Outdated Show resolved Hide resolved
@prathameshzarkar9 prathameshzarkar9 force-pushed the redundant-dockerfile-build-image-name branch 4 times, most recently from 9b75a1a to 16d9d10 Compare April 29, 2024 21:19
src/test/cli.build.test.ts Outdated Show resolved Hide resolved
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.

Thanks for the PR! Left a few comments.

src/spec-node/containerFeatures.ts Outdated Show resolved Hide resolved
.vscode/launch.json Outdated Show resolved Hide resolved
src/test/cli.build.test.ts Outdated Show resolved Hide resolved
src/test/cli.build.test.ts Outdated Show resolved Hide resolved
src/test/cli.build.test.ts Outdated Show resolved Hide resolved
src/test/configs/example-push/.devcontainer.json Outdated Show resolved Hide resolved
@prathameshzarkar9 prathameshzarkar9 force-pushed the redundant-dockerfile-build-image-name branch from 9b10555 to ad05d1d Compare May 2, 2024 03:51
src/test/cli.build.test.ts Outdated Show resolved Hide resolved
src/test/cli.build.test.ts Outdated Show resolved Hide resolved
src/test/cli.build.test.ts Show resolved Hide resolved
src/test/cli.build.test.ts Outdated Show resolved Hide resolved
@prathameshzarkar9 prathameshzarkar9 force-pushed the redundant-dockerfile-build-image-name branch from ad05d1d to b80617f Compare May 6, 2024 16:07
@chrmarti
Copy link
Contributor

chrmarti commented May 7, 2024

@prathameshzarkar9 I see you removed the test. Our hope was that we could have the test just without the --push option. Are you going to add it back? Thanks.

@prathameshzarkar9 prathameshzarkar9 force-pushed the redundant-dockerfile-build-image-name branch from 92e51fa to de4d1bf Compare May 14, 2024 06:01
@prathameshzarkar9 prathameshzarkar9 force-pushed the redundant-dockerfile-build-image-name branch from de4d1bf to f90f8a7 Compare May 15, 2024 02:40
src/test/cli.build.test.ts Outdated Show resolved Hide resolved
src/test/cli.build.test.ts Outdated Show resolved Hide resolved
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 good to me!

@chrmarti Can you help take another peek? Thanks!

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, thanks!

@chrmarti chrmarti enabled auto-merge (squash) May 27, 2024 10:11
@chrmarti chrmarti merged commit 6b04427 into devcontainers:main May 27, 2024
19 checks passed
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