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

feat: add token flag #79

Merged
merged 3 commits into from
Oct 30, 2023
Merged

feat: add token flag #79

merged 3 commits into from
Oct 30, 2023

Conversation

pottekkat
Copy link
Contributor

@pottekkat pottekkat commented Oct 23, 2023

Description

Adds a --token flag to pass the token in non-interactive environments.

Also hides the token input in a standard way by not showing anything in the terminal when a user inputs the token during the prompt.

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

@lingsamuel I seem to get the same errors when I run the tests from master branch as well.

@lingsamuel
Copy link
Contributor

let me check it

cmd/configure.go Outdated
@@ -168,11 +176,11 @@ func saveConfiguration(cmd *cobra.Command) error {

if rootConfig.Token == "" || overwrite {
fmt.Println("Please enter the APISIX token: ")
token, err := reader.ReadString('\n')
byteToken, err := term.ReadPassword(0)
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. if is terminal, use syscall.Stdin instead of 0
  2. if not, handle tty is necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to use syscall.Stdin.

@lingsamuel
Copy link
Contributor

@lingsamuel I seem to get the same errors when I run the tests from master branch as well.

No. master CI is working as expected. Maybe some dependency conflict in this PR.

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

No. master CI is working as expected. Maybe some dependency conflict in this PR.

Anything obvious?

@lingsamuel
Copy link
Contributor

No. master CI is working as expected. Maybe some dependency conflict in this PR.

Anything obvious?

no

@pottekkat
Copy link
Contributor Author

pottekkat commented Oct 24, 2023

It seems like the issue is with the hidden input for the token. Can I move that change to a separate PR?

@pottekkat pottekkat changed the title feat: add token flag and hide token input feat: add token flag Oct 25, 2023
@pottekkat
Copy link
Contributor Author

pottekkat commented Oct 25, 2023

@lingsamuel I have added just the token flag in this PR. Will move the other changes to a new PR.

@pottekkat pottekkat marked this pull request as draft October 25, 2023 17:21
@pottekkat
Copy link
Contributor Author

Wait till #85

@lingsamuel
Copy link
Contributor

TTY should be handled in this PR. Otherwise this PR is actually a breaking change because the users cannot configure adc without user-input anymore.

Signed-off-by: Navendu Pottekkat <navendu@apache.org>
@pottekkat pottekkat marked this pull request as ready for review October 26, 2023 10:31
@pottekkat
Copy link
Contributor Author

@lingsamuel Thank you for pointing that out. Added a check for terminals, and if not, fall back to the previous way we handled the input. The tests are passing, so it should be backward compatible now, right?

@lingsamuel lingsamuel merged commit faf0547 into main Oct 30, 2023
8 checks passed
@pottekkat pottekkat deleted the feat/support-token-flag/1 branch October 30, 2023 09:15
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.

None yet

2 participants