Add global registration bats tests for Katello clients #1391

ehelms · 2021-07-21T15:27:18Z

No description provided.

ehelms · 2021-07-21T15:29:14Z

bats/fb-katello-client-global-registration.bats

+  echo "rc=${status}"
+  echo "${output}"
+
+  registration_command=$(curl https://admin:changeme@$HOSTNAME/api/registration_commands -X POST -H 'Content-Type: application/json' -d '"activation_key":"${ACTIVATION_KEY}","organization":"${ORGANIZATION_LABEL}"' | ruby -e "require 'json'; puts JSON.load(ARGF.read)['registration_command']")


This appears to not work as it generates the registration command with Default_Organization instead of Test_Organization not respecting passing the organization label? Is this a bug or am I setting this wrong? @ares @stejskalleos

Additionally, I noticed that if I pass a parameter that registeration does not support there is not an error, the parameter is just ignored. That led to some debugging confusion

For what it's worth, you shouldn't have to post using JSON. You can also shorten the Ruby line a bit.

Suggested change

registration_command=$(curl https://admin:changeme@$HOSTNAME/api/registration_commands -X POST -H 'Content-Type: application/json' -d '"activation_key":"${ACTIVATION_KEY}","organization":"${ORGANIZATION_LABEL}"' | ruby -e "require 'json'; puts JSON.load(ARGF.read)['registration_command']")

registration_command=$(curl https://admin:changeme@$HOSTNAME/api/registration_commands -X POST -d "activation_key=${ACTIVATION_KEY}" -d "organization=${ORGANIZATION_LABEL}" | ruby -rjson -e "puts JSON.load(ARGF.read)['registration_command']")

I think you need to pass IDs, not names/labels for org?

https://github.com/theforeman/foreman/blob/5a66daa44921e63522bb90d01f8924b6cd4abcd5/app/controllers/concerns/foreman/controller/registration.rb#L17-L20
https://github.com/theforeman/foreman/blob/5a66daa44921e63522bb90d01f8924b6cd4abcd5/app/controllers/api/v2/registration_commands_controller.rb#L9-L21

Additionally, I noticed that if I pass a parameter that registeration does not support there is not an error, the parameter is just ignored. That led to some debugging confusion

What if I told you, ALL of our API behaves like this, and I might have gray hair because of that?

Yea... @ares was saddening me with this. Just makes debugging that much harder in this case to know what my client is doing.

I think you need to pass IDs, not names/labels for org?

Yea... for this to work right now I will have to do that. I left it like to to first help the conversation about adding name/label support for organizations.

I can file an RFE for the label for Katello. Should name work though?

I can file an RFE for the label for Katello. Should name work though?

Oh I guess not from what @evgeni linked. I'll file an issue for that too.

I would call it an RFE :-) I guess we shouldn't block the test on it. Note that if you insisht on org label and not the name, it must be added from Katello, since label is a katello thing. Perhaps it's time to move this extension to Foreman core where it belongs to?

By extension, you mean the concept of a label on organization to Foreman core?

I filed the following for consideration:

https://projects.theforeman.org/issues/33227

https://projects.theforeman.org/issues/33228

For what it's worth, you shouldn't have to post using JSON. You can also shorten the Ruby line a bit.

The API only accepts JSON when using POST.

ehelms · 2021-07-28T19:46:47Z

roles/bats/defaults/main.yaml

@@ -17,6 +17,7 @@ bats_tests:
  - "fb-test-katello.bats"
  - "fb-katello-content.bats"
  - "fb-katello-client.bats"
+  - "fb-katello-client-global-registration.bats"


I probably need to not include this for some versions of Foreman and Katello we currently support. What those would be, I am not presently sure of.

4.0+ I believe

ares

Great to see we'll have test for this, do we only keep happy paths here or can we add more variants? It would be awesome to have a test that also creates PAT (personal access token) for a user (e.g. admin) and then we get the registration command using that instead of changeme password. OTOH if we already have test for PAT elsewhere, ignore me :-)

ehelms · 2021-08-06T17:48:33Z

This has been updated to use organization_id for now while we work out what to do about name and label. This only adds tests for Katello at the moment and aims to mimic the basics of the existing client bats tests. I opened an issue to track adding additional global registration tests: #1396

My goal is to get basic coverage for the Katello scenario to help ensure the new workflow works as we aim to replace the old one and then in subsequent PRs layer in new test suites.

ehelms · 2021-08-11T13:25:58Z

@jturel @stejskalleos Could you both take a look to ensure this looks OK and that it captures a workflow you'd expect?

ares · 2021-08-12T13:33:40Z

Makes sense to me, IMHO good to go in (didn't perform the syntax check, just the semantics) 👍

jturel · 2021-08-12T13:51:56Z

Looks good to me :)

ehelms · 2021-08-18T22:03:11Z

@evgeni Mind giving this a look?

evgeni

Two comments, but willing to give it a try as-is and cleanup the broken pieces later.

(Assuming you have run this against nightly and it passes 😅)

evgeni · 2021-08-24T15:35:51Z

bats/fb-katello-client-global-registration.bats

+  fi
+
+  cleanSubscriptionManager
+  tPackageRemove subscription-manager


Is that so that the GR command can install it again?

Doesn't this break pulp3 on EL8 where this might kill python3-rhsm (or whatever the right pkg name is)?

I hope not. I did not see any breaks with my testing.

If you ran it on EL8 and it did not make pulp-certguard go belly up, we're good.

Looks all good:

========================================================================================================================================================================================================================================================================================== Package Architecture Version Repository Size ========================================================================================================================================================================================================================================================================================== Removing: subscription-manager x86_64 1.28.13-3.el8_4 @baseos 4.3 M Removing unused dependencies: dnf-plugin-subscription-manager x86_64 1.28.13-3.el8_4 @baseos 87 k python3-dmidecode x86_64 3.12.2-15.el8 @baseos 308 k python3-ethtool x86_64 0.14-3.el8 @baseos 92 k python3-inotify noarch 0.9.6-13.el8 @baseos 243 k python3-librepo x86_64 1.12.0-3.el8 @baseos 172 k usermode x86_64 1.113-1.el8 @baseos 837 k Transaction Summary ========================================================================================================================================================================================================================================================================================== Remove 7 Packages

evgeni · 2021-08-24T15:36:51Z

bats/fb-katello-client-global-registration.bats

+  tPackageInstall walrus-0.71 && tPackageExists walrus-0.71
+}
+
+@test "check available errata" {


Feels like this duplicates the original content tests -- might want to move it to a helper later?

Yea, can do, as I have another test suite in mind that would do the same thing.

ehelms commented Jul 21, 2021

View reviewed changes

ehelms mentioned this pull request Jul 21, 2021

Fixes #32678 - katello_ca_consumer in registration template theforeman/foreman#8563

Merged

ehelms commented Jul 28, 2021

View reviewed changes

ares reviewed Aug 5, 2021

View reviewed changes

ehelms force-pushed the add-katello-client-global-registration branch from f9eaa72 to cf22743 Compare August 6, 2021 16:49

Add global registration bats tests for Katello clients

8fc2e8f

ehelms force-pushed the add-katello-client-global-registration branch from cf22743 to 8fc2e8f Compare August 6, 2021 17:45

ehelms requested review from evgeni and ares August 24, 2021 15:17

evgeni approved these changes Aug 24, 2021

View reviewed changes

ehelms merged commit b07f7d6 into theforeman:master Aug 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add global registration bats tests for Katello clients #1391

Add global registration bats tests for Katello clients #1391

ehelms commented Jul 21, 2021

ehelms Jul 21, 2021

ekohl Jul 21, 2021

evgeni Jul 22, 2021 •

edited

Loading

evgeni Jul 22, 2021

ehelms Jul 22, 2021

ehelms Aug 5, 2021

ehelms Aug 6, 2021

ehelms Aug 6, 2021

ehelms Aug 6, 2021

ehelms Aug 6, 2021

ehelms Jul 28, 2021

jturel Aug 12, 2021

ares left a comment

ehelms commented Aug 6, 2021

ehelms commented Aug 11, 2021

ares commented Aug 12, 2021

jturel commented Aug 12, 2021

ehelms commented Aug 18, 2021

evgeni left a comment

evgeni Aug 24, 2021

ehelms Aug 24, 2021

evgeni Aug 24, 2021

ehelms Aug 24, 2021

evgeni Aug 24, 2021

ehelms Aug 24, 2021

	registration_command=$(curl https://admin:changeme@$HOSTNAME/api/registration_commands -X POST -H 'Content-Type: application/json' -d '"activation_key":"${ACTIVATION_KEY}","organization":"${ORGANIZATION_LABEL}"' \| ruby -e "require 'json'; puts JSON.load(ARGF.read)['registration_command']")
	registration_command=$(curl https://admin:changeme@$HOSTNAME/api/registration_commands -X POST -d "activation_key=${ACTIVATION_KEY}" -d "organization=${ORGANIZATION_LABEL}" \| ruby -rjson -e "puts JSON.load(ARGF.read)['registration_command']")

Add global registration bats tests for Katello clients #1391

Add global registration bats tests for Katello clients #1391

Conversation

ehelms commented Jul 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evgeni Jul 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ares left a comment

Choose a reason for hiding this comment

ehelms commented Aug 6, 2021

ehelms commented Aug 11, 2021

ares commented Aug 12, 2021

jturel commented Aug 12, 2021

ehelms commented Aug 18, 2021

evgeni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evgeni Jul 22, 2021 •

edited

Loading