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

Feature addreg code ways #268

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

paragjain0910
Copy link

@paragjain0910 paragjain0910 commented Oct 7, 2024

SUSE Connect - Registration Code Input Handling

New Enhancement Added

Support for Two New Ways to Pass Registration Codes

  1. Standard Input (stdin):

    • By using -r -, you can pass the registration code directly from stdin.
    • Example:
      echo "1234" | go run suseconnect.go -r -
  2. File Input:

    • By using -r @/file/path, you can specify a file path that contains the registration code. The current directory is considered to be same as of the suseconnect.go file. Pass the path reference accordingly.
    • Example:
      go run suseconnect.go -r @/my/file

processToken Function Documentation

The processToken function is a part of the suseconnect package located in the cmd/suseconnect/suseconnect.go file. This function is responsible for processing the token input. The token can be:

  • A string (the registration code directly),
  • A file path prefixed with @, or
  • - indicating that the registration code should be read from stdin.

Added the Test cases as well.

Function Signature

func processToken(token string) (string, error)

@felixsch
Copy link
Contributor

felixsch commented Oct 7, 2024

@mssola Do we plan to have the cli implementation reworked? If this is the case (what I hope) we should maybe wait if this functionality is not provided by the library itself (idk, https://github.com/urfave/cli)

What do you think @paragjain0910?

@mssola
Copy link
Contributor

mssola commented Oct 7, 2024

@mssola Do we plan to have the cli implementation reworked? If this is the case (what I hope) we should maybe wait if this functionality is not provided by the library itself (idk, https://github.com/urfave/cli)

That is true... That being said, when do we want to do such a thing? Is there any epic you think that could work out? If so, let's talk about it during the workshop next week and plan things.

@paragjain0910
Copy link
Author

paragjain0910 commented Oct 9, 2024

I was going through the documentation of urfave. Here the key points :

  1. It seems to be a more declarative way to simply managing commands, subcommands and flags.
  2. Considering the performance and the size of the urfave binary, I feel there will be no significant performance impact.
  3. It can easily support existing cli features like such as aliases, subcommands, and flexible argument handling like -r @/filepath, -r -, and -r 1234)
  4. If there are plans to scale suseconnect-ng or add more complex CLI interactions in the future, refactoring with urfave/cli could save time and effort down the line.

Also I tried creating a sample go file using urfave for supporting the above registration code features. @felixsch @mssola you can also have a look once, maybe it can help in reflecting upon the effort required to bring the change.
main.txt

Given these points, I would recommend reworking the CLI implementation using urfave/cli.

@mssola
Copy link
Contributor

mssola commented Oct 11, 2024

@paragjain0910 thanks for the pointers. I have had some experience with https://github.com/urfave/cli in the past (e.g. back when I was involved with https://github.com/SUSE/zypper-docker, even if that was an old version). So far I believe that it would be the right choice.

That being said, this would be a big change and I believe that the workshop next week can clarify when and how to apply this change. Moreover, I'd even envision a SUSEConnect which can break backwards compatibility, and have a proper action-like system in which you would SUSEConnect register <blabla> instead of the mess we have right now.

So, food for thought, @felixsch 😄

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.

3 participants