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.

Sign up for GitHub

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

Jump to bottom

feat: enable render in skaffold run v2. #6761

Merged
merged 1 commit into from
Nov 8, 2021
Merged

feat: enable render in skaffold run v2. #6761

merged 1 commit into from
Nov 8, 2021

Conversation

yuwenma
Copy link
Contributor

@yuwenma yuwenma commented Oct 24, 2021 •
edited
Loading

Related: #5673

Description
Since render is necessary in the new skaffold pipeline. skaffold run should have render enabled.

@yuwenma yuwenma requested a review from a team as a code owner October 24, 2021 01:16
@yuwenma yuwenma requested review from aaron-prindle and removed request for a team October 24, 2021 01:16
@google-cla google-cla bot added the cla: yes label Oct 24, 2021
@yuwenma yuwenma requested review from nkubala and removed request for aaron-prindle October 24, 2021 01:18
@yuwenma yuwenma changed the title feat: Enable render in skaffold run v2. feat: enable render in skaffold run v2. Oct 24, 2021
@yuwenma yuwenma requested a review from tejal29 October 24, 2021 01:20
nkubala
nkubala reviewed Oct 25, 2021
View reviewed changes
Copy link
Contributor

@nkubala nkubala left a comment

Choose a reason for hiding this comment

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

this looks good - is there an existing integration test that would have caught this if it was enabled?

@yuwenma
Copy link
Contributor Author

yuwenma commented Oct 25, 2021

this looks good - is there an existing integration test that would have caught this if it was enabled?

I'm not sure. But I think we should have such a test. I'm not quite familiar with the deploy part of run and apply. IIUC, the run and apply have the same render and deploy logic, while dev is different since it detects changes and re-apply steps affected by the changed manifests.

@briandealwis
Copy link
Member

Looks to be failing in the same way as #6712.

@yuwenma yuwenma force-pushed the enable-render-in-run branch from ac48c6d to a3d6366 Compare November 8, 2021 18:14
@yuwenma yuwenma force-pushed the enable-render-in-run branch from 0c1dfb8 to a3d6366 Compare November 8, 2021 18:16
@pull-request-size pull-request-size bot added size/L and removed size/S labels Nov 8, 2021
@yuwenma
feat: Enable render in skaffold run v2.
58dca40 
 Since render is necessary in the new skaffold pipeline. `skaffold run` should have render enabled.
@yuwenma yuwenma force-pushed the enable-render-in-run branch from a3d6366 to 58dca40 Compare November 8, 2021 18:21
@pull-request-size pull-request-size bot added size/S and removed size/L labels Nov 8, 2021
@nkubala nkubala merged commit b3aa4e9 into GoogleContainerTools:v2 Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nkubala nkubala nkubala approved these changes

@tejal29 tejal29 Awaiting requested review from tejal29

Assignees

@nkubala nkubala

Labels
cla: yes size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yuwenma @briandealwis @nkubala