Skip to content

Conversation

@iyear
Copy link
Member

@iyear iyear commented Jul 13, 2022

Pre-Checklist

Note: please complete ALL items in the following checklist.

  • I have read through the CONTRIBUTING.md documentation.
  • My code has the necessary comments and documentation (if needed).
  • I have added relevant tests

Description

I remove the default value of template. We can discuss better solution.

Related Issues

close #841

New Behavior (screenshots if needed)

image

@iyear iyear requested a review from a team as a code owner July 13, 2022 07:08
@iyear
Copy link
Member Author

iyear commented Jul 13, 2022

In #795 , @IronCore864 thinks that templates are less suitable to be exported by command, but are suitable to be placed under /examples. The user gets the yaml file as a download.

Now two concurrent quickstart.yaml and gitops.yaml are preventing maintainability. If a solution can be discussed, I can fix it in the meantime.

@daniel-hutao @aFlyBird0

@daniel-hutao
Copy link
Member

In #795 , @IronCore864 thinks that templates are less suitable to be exported by command, but are suitable to be placed under /examples. The user gets the yaml file as a download.

Now two concurrent quickstart.yaml and gitops.yaml are preventing maintainability. If a solution can be discussed, I can fix it in the meantime.

@daniel-hutao @aFlyBird0

@iyear I've left a comment under #795. Let's have more discussions there.

@daniel-hutao
Copy link
Member

For quickstart printing logic, my original design was "when -p does not exist, -t defaults to equal quickstart when -t is not set; when -p exists, -t does not take effect".

We can follow this idea to fix it first. As for whether to delete the logic of quickstart configuration printing, we can wait until the end of the #795 discussion and implement it in a separate pr. @iyear

Copy link
Member

@daniel-hutao daniel-hutao left a comment

Choose a reason for hiding this comment

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

@iyear
Copy link
Member Author

iyear commented Jul 15, 2022

@daniel-hutao

plugin := viper.GetString("plugin")
if plugin == "" {
fmt.Println(DefaultConfig)

Then I have to remove this code.
Only one of the -p and -t args can be allowed to carry a default value.

@daniel-hutao
Copy link
Member

I looked at the code and we need to prioritize support for default config printing, i.e. dtm show config can only print internal/pkg/show/config/default.yaml. so we had to drop the default printing logic for quickstart. So, let's just keep the -t quickstart support.

@iyear Please also check if the documentation needs to be updated.

Releated issue: #597

@iyear
Copy link
Member Author

iyear commented Jul 19, 2022

@daniel-hutao Is it possible to review and then merge in? I will start working on #795, otherwise it will cause a conflict.

Signed-off-by: iyear <ljyngup@gmail.com>
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.

💣 Bug: command dtm show config -p harbor print the wrong config sample

3 participants