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

docs(cli): Add a long description for file render #1007

Merged
merged 4 commits into from
Sep 22, 2023
Merged

Conversation

lena-larionova
Copy link
Contributor

The existing description for deck file render doesn't currently provide any detail on what the command does or how to use it.
I was trying to test it out and couldn't even figure out how it was supposed to work, since there's no input file flag or example.

Found the info I needed on Slack here, and based the update on that: https://kongstrong.slack.com/archives/C04349E4KRC/p1690921140291889

❗ I did run into one potential issue while testing: this command doesn't warn you when overwriting existing files, like deck dump or others do. Is that intentional?

@lena-larionova lena-larionova requested review from a team and rspurgeon August 21, 2023 22:58
@rspurgeon
Copy link
Collaborator

cc. @zekth

cmd/file_render.go Outdated Show resolved Hide resolved
zekth
zekth previously approved these changes Sep 5, 2023
@Tieske
Copy link
Member

Tieske commented Sep 5, 2023

@zekth iirc this command also renders the files, as they would be synced to the control plane (kong or Konnect), such that it will also do substitutions etc. That is currently not in the description, should it be? including maybe a warning that secrets might be in the rendered file?

@zekth
Copy link
Member

zekth commented Sep 5, 2023

currently render use mocks for values, so secrets won't be rendered.

ref:

func GetContentFromFiles(filenames []string, mockEnvVars bool) (*Content, error) {

true)

@rspurgeon
Copy link
Collaborator

@lena-larionova @zekth can we get this updated with the one suggestion (or something similar) and get it merged?

@lena-larionova
Copy link
Contributor Author

this command also renders the files, as they would be synced to the control plane (kong or Konnect), such that it will also do substitutions etc.

How's this @Tieske @zekth :
Short description: "Combines multiple complete configuration files and renders them as one Kong declarative config file."

Then some questions about the rest:

currently render use mocks for values, so secrets won't be rendered.

Does this mean the rendered file isn't in a ready state for syncing to a control plane, as the user has to replace mocked values with their own? Or am I misunderstanding something.

such that it will also do substitutions etc. That is currently not in the description, should it be?

What should be said about substitutions? What's the etc part? Is that something we should list out? What exactly should we add to the long description here?

@Tieske
Copy link
Member

Tieske commented Sep 6, 2023

currently render use mocks for values, so secrets won't be rendered.

Does this mean the rendered file isn't in a ready state for syncing to a control plane, as the user has to replace mocked values with their own? Or am I misunderstanding something.

No, the goal is to validate local, so users would not edit manually, but if successful do an actual sync with actual values (not mocked ones).

Deck does;

  1. grab files
  2. combine them
  3. render variables and "other magic"
  4. sync to CP

especially step 3 & 4 were a single operation, without an option to inspect the output of step 3. The new render command allows a user to execute the same steps, but without the sync, so they can validate locally. (otherwise the user would need a running Kong system to test the sync against)

such that it will also do substitutions etc. That is currently not in the description, should it be?

What should be said about substitutions? What's the etc part? Is that something we should list out? What exactly should we add to the long description here?

Above I named it "other magic" because I do not know exactly what deck logic is executed here. Probably @GGabriele knows. If there is any additional logic, and if so, what it exactly is.

@rspurgeon
Copy link
Collaborator

I think my suggestion above covers enough of the use case here to apply it and merge this down

Copy link
Collaborator

@rspurgeon rspurgeon left a comment

Choose a reason for hiding this comment

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

left a suggestion

cmd/file_render.go Outdated Show resolved Hide resolved
cmd/file_render.go Outdated Show resolved Hide resolved
cmd/file_render.go Outdated Show resolved Hide resolved
@lena-larionova
Copy link
Contributor Author

@zekth @Tieske Added Rick's suggestion, how does that look?

Also added the term "render" back into the short description. That makes the description a bit long - is there a character limit I need to be aware of?

Copy link
Collaborator

@rspurgeon rspurgeon left a comment

Choose a reason for hiding this comment

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

LGTM

zekth
zekth previously approved these changes Sep 19, 2023
Copy link
Member

@zekth zekth left a comment

Choose a reason for hiding this comment

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

thanks

@codecov-commenter
Copy link

Codecov Report

Patch coverage has no change and project coverage change: -0.05% ⚠️

Comparison is base (6336929) 33.78% compared to head (2e2a096) 33.74%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1007      +/-   ##
==========================================
- Coverage   33.78%   33.74%   -0.05%     
==========================================
  Files         101      101              
  Lines       12360    12375      +15     
==========================================
  Hits         4176     4176              
- Misses       7775     7790      +15     
  Partials      409      409              
Files Changed Coverage Δ
cmd/file_render.go 0.00% <0.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Tieske Tieske requested a review from GGabriele September 21, 2023 15:38
@GGabriele GGabriele merged commit 677198b into main Sep 22, 2023
34 checks passed
@GGabriele GGabriele deleted the docs/file-render branch September 22, 2023 13:41
AntoineJac pushed a commit that referenced this pull request Jan 23, 2024
---------

Co-authored-by: Rick Spurgeon <[email protected]>
Co-authored-by: Thijs Schreijer <[email protected]>
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.

6 participants