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

Deprecate --render-only and --render-output flags #5644

Merged
merged 1 commit into from
Apr 14, 2021

Conversation

nkubala
Copy link
Contributor

@nkubala nkubala commented Apr 7, 2021

This change deprecates the --render-only and --render-output flags from skaffold run, in favor of using the skaffold render command directly.

@nkubala nkubala requested a review from a team as a code owner April 7, 2021 23:12
@google-cla google-cla bot added the cla: yes label Apr 7, 2021
@codecov
Copy link

codecov bot commented Apr 7, 2021

Codecov Report

Merging #5644 (dcdff66) into master (1ef4fd8) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head dcdff66 differs from pull request most recent head c364427. Consider uploading reports for the commit c364427 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5644   +/-   ##
=======================================
  Coverage   70.65%   70.66%           
=======================================
  Files         411      411           
  Lines       15793    15798    +5     
=======================================
+ Hits        11159    11164    +5     
  Misses       3813     3813           
  Partials      821      821           
Impacted Files Coverage Δ
cmd/skaffold/app/cmd/flags.go 89.02% <100.00%> (+0.13%) ⬆️
pkg/skaffold/runner/runner.go 0.00% <0.00%> (ø)
pkg/skaffold/runner/build_deploy.go 69.00% <0.00%> (+0.31%) ⬆️
pkg/skaffold/deploy/helm/deploy.go 75.82% <0.00%> (+0.34%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ef4fd8...c364427. Read the comment docs.

Copy link
Member

@tejal29 tejal29 left a comment

Choose a reason for hiding this comment

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

Is there a way we can show the deprecated warning in command help?

cmd/skaffold/app/cmd/flags.go Outdated Show resolved Hide resolved
cmd/skaffold/app/cmd/flags.go Outdated Show resolved Hide resolved
@nkubala
Copy link
Contributor Author

nkubala commented Apr 8, 2021

Is there a way we can show the deprecated warning in command help?

actually, using the MarkDeprecated() method from pflag explicitly removes the flag from any help or usage messages from now on, which makes sense because we don't really want anyone using this flag from here on out. this will only show up for people who are already using the flag.

@tejal29
Copy link
Member

tejal29 commented Apr 9, 2021

Is there a way we can show the deprecated warning in command help?

actually, using the MarkDeprecated() method from pflag explicitly removes the flag from any help or usage messages from now on, which makes sense because we don't really want anyone using this flag from here on out. this will only show up for people who are already using the flag.

That is strange. The command help docs did not change in this PR.

@tejal29 tejal29 added the docs-modifications runs the docs preview service on the given PR label Apr 9, 2021
@container-tools-bot
Copy link

Please visit http://35.236.21.86:1313 to view changes to the docs.

@container-tools-bot container-tools-bot removed the docs-modifications runs the docs preview service on the given PR label Apr 9, 2021
@nkubala nkubala merged commit e80d6f0 into GoogleContainerTools:master Apr 14, 2021
@nkubala nkubala deleted the deprecate-render-only branch April 14, 2021 19:15
@nkubala nkubala added this to the v1.24.0 milestone May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants