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: initial run of adc configure does not throw error #85

Merged
merged 5 commits into from
Oct 30, 2023

Conversation

pottekkat
Copy link
Contributor

@pottekkat pottekkat commented Oct 25, 2023

Description

Handling a different scenario just for the config command is different. This approach makes it better by showing warnings indicating the lack of a configuration file. Now commands that require a configuration file ask users to run adc configure, and if the configuration file is not present, it is shown as a warning.

In the adc configure subcommand, when it is run initially without the configuration file being present, will show that the configuration file is not present and then proceed to create one without needing the -f flag to overwrite the created file.

➜  adc sync
Configuration file /Users/navendu/.adc.yaml doesn't exist.
ADC isn't configured, run `adc configure` to configure ADC.
➜  adc configure
Configuration file /Users/navendu/.adc.yaml doesn't exist.
Please enter the APISIX server address: 
http://127.0.0.1:9081
Please enter the APISIX token: 
token
ADC configured successfully!

Fixes #83

The other alternative was to create a new configuration file if it is not present and then write to it properly if it has just been initialized, but this is a more simple and central solution.

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible

Signed-off-by: Navendu Pottekkat <navendu@apache.org>
@pottekkat
Copy link
Contributor Author

The failing tests seem to pass when I run locally.

Screenshot 2023-10-25 at 19 12 41

Signed-off-by: Navendu Pottekkat <navendu@apache.org>
@pottekkat
Copy link
Contributor Author

pottekkat commented Oct 25, 2023

So the issue was because WriteConfig fails to write when the file is not present. Since with this change, we stop creating a new file if it does not exist, the WriteConfig function fails. There is an issue open in Viper to change this behaviour: spf13/viper#433

But this does seem to be fixed. As a workaround, we can use the WriteConfigAs function. This will make sure that the existing tests pass and the UX is correct.

This UX remains the same as before:

adc configure --address=https://127.0.0.1:9180 -f
Configuration file /Users/navendu/.adc.yaml doesn't exist.
Please enter the APISIX token: 
token
ADC configured successfully!

Without this change, if I run this, I get this error, which is why the CI was failing before:

adc configure --address=https://127.0.0.1:9180 -f
Configuration file /Users/navendu/.adc.yaml doesn't exist.
Please enter the APISIX token: 
token
Failed to configure ADC
Error: Config File ".adc" Not Found in "[/Users/navendu]"

My previous comment that it was passing when tested locally was because I already had the .adc.yaml file present in my home directory. So, my bad!

@pottekkat pottekkat mentioned this pull request Oct 25, 2023
5 tasks
@pottekkat pottekkat requested a review from starsz October 26, 2023 05:44
cmd/root.go Outdated Show resolved Hide resolved
Signed-off-by: Navendu Pottekkat <navendu@apache.org>
Signed-off-by: Navendu Pottekkat <navendu@apache.org>
cmd/root.go Outdated Show resolved Hide resolved
Signed-off-by: Navendu Pottekkat <navendu@apache.org>
@lingsamuel lingsamuel merged commit 0b9e56c into main Oct 30, 2023
8 checks passed
@pottekkat pottekkat deleted the bug/config-file-init/1 branch October 30, 2023 05:02
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: Running adc configure for the first time fails because "config file already exists"
2 participants