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.

Sign up for GitHub

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

Jump to bottom

feat: Big refactoring of main.go #20

Merged
merged 2 commits into from
Nov 18, 2022
Merged

feat: Big refactoring of main.go #20

merged 2 commits into from
Nov 18, 2022

Conversation

jirihnidek
Copy link
Contributor

  • Functions of sub-commands were moved to function outside of main function. Created several functions to not duplicate code. Side effect of this effort is better displaying of errors for "connect" sub-command.
  • Functions for "main" and "before" actions were also move outside main function
  • Fixed one small issue with registration to RHSM. It is possible to use --organization, when --username and --password are specified. The registration process could be improved more, but it should be part of another commit. Added only fix-me comment for this case.

@jirihnidek
Copy link
Contributor Author

Note: connecting process could look like this:

[root@thinkpad-t580 rhc]# ./rhc connect --username admin --password admin --organization donaldduck
Connecting thinkpad-t580 to Red Hat.
This might take a few seconds.

● Connected to Red Hat Subscription Management
● Connected to Red Hat Insights
! Cannot activate the rhc daemon

Manage your Red Hat connector systems: https://red.ht/connector

The following errors were encountered during connect:

STEP  ERROR  
rhc   cannot activate daemon: Unit file rhcd.service does not exist.

subpop
subpop requested changes Nov 1, 2022
View reviewed changes
Copy link
Collaborator

@subpop subpop left a comment

Choose a reason for hiding this comment

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

Nicely executed! This is a great improvement, and sets us up for further refactoring as this program matures.

main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
jirihnidek
jirihnidek commented Nov 2, 2022
View reviewed changes
main.go Show resolved Hide resolved
@jirihnidek jirihnidek added the work in progress More issues have to be resolved before it is ready for review label Nov 2, 2022
jirihnidek
jirihnidek commented Nov 2, 2022
View reviewed changes
main.go Outdated Show resolved Hide resolved
@jirihnidek
feat: Big refactoring of main.go
7051a91 
* Functions of sub-commands were moved to function outside of
  main function. Created several functions to not duplicate
  code. Side effect of this effort is better displaying
  of errors for "connect" sub-command.
* Functions for "main" and "before" actions were also move
  outside main function
* Some constants were removed (better description)
* Added some documentation comments to functions
* Fixed one small issue with registration to RHSM. It is
  possible to use --organization, when --username and
  --password are specified. The registration process could
  be improved more, but it should be part of another commit.
  Added only fix-me comment for this case.
@jirihnidek
Copy link
Contributor Author

The connection looks like this, when wrong password is provided:

[root@rhel9 rhc]# ./rhc connect --username REDACTED --password foo
Connecting rhel9 to Red Hat.
This might take a few seconds.

! Cannot connect to Red Hat Subscription Management
● Skipping connection to Red Hat Insights
● Skipping activation of rhc daemon

Manage your Red Hat connector systems: https://red.ht/connector

The following errors were encountered during connect:

STEP  ERROR  
rhsm  cannot connect to Red Hat Subscription Management: error: Invalid username or password. To create a login, please visit https://www.redhat.com/wapps/ugc/register.html

@jirihnidek jirihnidek removed the work in progress More issues have to be resolved before it is ready for review label Nov 2, 2022
@jirihnidek
Copy link
Contributor Author

@subpop Hi, can you please review this PR once again? Thanks in advance.

@jirihnidek
Copy link
Contributor Author

BTW: created BZ for one issue fixed as part of this PR: https://bugzilla.redhat.com/show_bug.cgi?id=2142797

@jirihnidek jirihnidek mentioned this pull request Nov 16, 2022
Merged
subpop
subpop approved these changes Nov 18, 2022
View reviewed changes
Copy link
Collaborator

@subpop subpop left a comment

Choose a reason for hiding this comment

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

Looks great! I found what looked like one typo in a comment. Otherwise, I'm ready to merge this.

main.go Outdated Show resolved Hide resolved
@subpop
docs: correct service name in statusAction doc
ffcdabb 
The doc comment in statusAction incorrectly referenced 'rhsm.service'.
@subpop subpop merged commit 2091c4b into RedHatInsights:main Nov 18, 2022
@jirihnidek jirihnidek deleted the refactoring_main branch November 21, 2022 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@subpop subpop subpop approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jirihnidek @subpop