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

fix: kpt force named "false" in schema #9074

Merged
merged 2 commits into from
Oct 25, 2023
Merged

fix: kpt force named "false" in schema #9074

merged 2 commits into from
Oct 25, 2023

Conversation

zevisert
Copy link
Contributor

@zevisert zevisert commented Sep 7, 2023

Fixes: None
Related: None
Merge before/after: None
^ I didn't create an issue, I just edited directly on github 🥲

Description

I think the intent for the current schema at skaffold.yaml:deploy.kpt.false was meant to be deploy.kpt.force.
https://skaffold.dev/docs/references/yaml/#deploy-kpt-false

User facing changes (remove if N/A)

Users using deploy.kpt.false will need to change to deploy.kpt.force.

Follow-up Work (remove if N/A)

  • I don't know how skaffold fix works, this could be auto fixed.

@zevisert
Copy link
Contributor Author

Anything I can do to merge this? It's been sitting open for a while now. I could rebase, and I could also look into skaffold fix

@zevisert
Copy link
Contributor Author

Rebased on main. There is no need for any work in any pkg/skaffold/schema/<version>/upgrade.go for skaffold fix to function correctly, since this field was always parsed correctly, so the internal representation is/was correct.

$ cat << END > skaffold.yaml  
apiVersion: skaffold/v4beta1
kind: Config
metadata:
  name: PR9074
deploy:
  kpt:
    "false": true
END

$ ./out/skaffold fix
apiVersion: skaffold/v4beta8
kind: Config
metadata:
  name: PR9074
deploy:
  kpt:
    force: true

@renzodavid9 renzodavid9 added the kokoro:force-run forces a kokoro re-run on a PR label Oct 20, 2023
@kokoro-team kokoro-team removed the kokoro:force-run forces a kokoro re-run on a PR label Oct 20, 2023
@renzodavid9 renzodavid9 added the kokoro:force-run forces a kokoro re-run on a PR label Oct 23, 2023
@kokoro-team kokoro-team removed the kokoro:force-run forces a kokoro re-run on a PR label Oct 23, 2023
@renzodavid9
Copy link
Contributor

Hey @zevisert, thanks for opening this PR. Could you please rebase it? This will add your changes on top of the new schema version created. Also, according to the failing tests, you'll need to run make generate-schemas and make generate-schemas-v2, those commands will update the files used as a source to render the docs page. Thanks! and sorry for the delay here.

@zevisert
Copy link
Contributor Author

Rebased, and schemas updated. Thanks for interpreting the test results for me - I ran and reverted both commands a couple of times, it seemed like both had the same effect. I committed the updated schema.

No worries about the time taken, there's lots of other open issues here!

@zevisert
Copy link
Contributor Author

Rebased once more to sign commits

@renzodavid9 renzodavid9 added the kokoro:force-run forces a kokoro re-run on a PR label Oct 25, 2023
@kokoro-team kokoro-team removed the kokoro:force-run forces a kokoro re-run on a PR label Oct 25, 2023
@codecov
Copy link

codecov bot commented Oct 25, 2023

Codecov Report

Merging #9074 (98a3f94) into main (290280e) will decrease coverage by 7.12%.
Report is 1050 commits behind head on main.
The diff coverage is 48.33%.

@@            Coverage Diff             @@
##             main    #9074      +/-   ##
==========================================
- Coverage   70.48%   63.36%   -7.12%     
==========================================
  Files         515      630     +115     
  Lines       23150    32419    +9269     
==========================================
+ Hits        16317    20542    +4225     
- Misses       5776    10312    +4536     
- Partials     1057     1565     +508     
Files Coverage Δ
cmd/skaffold/app/cmd/commands.go 97.61% <100.00%> (+0.35%) ⬆️
cmd/skaffold/app/cmd/config.go 100.00% <100.00%> (ø)
cmd/skaffold/app/cmd/config/list.go 65.21% <ø> (ø)
cmd/skaffold/app/cmd/config/set.go 88.72% <ø> (ø)
cmd/skaffold/app/cmd/config/util.go 54.28% <ø> (ø)
cmd/skaffold/app/cmd/credits.go 100.00% <ø> (ø)
cmd/skaffold/app/cmd/debug.go 100.00% <100.00%> (ø)
cmd/skaffold/app/cmd/flags.go 93.00% <100.00%> (+2.18%) ⬆️
cmd/skaffold/app/cmd/generate_pipeline.go 60.00% <ø> (ø)
cmd/skaffold/app/cmd/init.go 100.00% <100.00%> (ø)
... and 41 more

... and 433 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@renzodavid9 renzodavid9 merged commit 6f12afe into GoogleContainerTools:main Oct 25, 2023
15 checks passed
@zevisert zevisert deleted the patch-1 branch February 16, 2024 00:01
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.

4 participants