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: Enable register with environment names & type #3480

Merged
merged 1 commit into from
Dec 19, 2024

Conversation

jirihnidek
Copy link
Contributor

@jirihnidek jirihnidek commented Dec 9, 2024

  • Allow to register also with environment names. It is possible
    to use environment names with username & password and
    activation-keys & organization authentication
  • It is not allowed to use environmets and environment_names
    registration options together, because it is not possible to use
    env. IDs and names together on candlepin server
  • Introduce environment_type as another registration_option
    • When consumer object is returned and it contains environments,
      then all environments are checked if type of environment
      matches given environment_type. If type is missing or is
      different, then system is unregistered and exception is
      raised
  • Modified few unit tests related to environments
  • Added few unit tests

@cnsnyder cnsnyder requested review from a team and jstavel and removed request for a team December 9, 2024 10:45
Copy link

github-actions bot commented Dec 9, 2024

Coverage

Coverage (computed on Fedora latest) •
FileStmtsMissCoverMissing
rhsm
   connection.py100746154%48–49, 53, 55–56, 81, 95, 147, 281, 312, 378–383, 387–396, 457, 459, 565, 568, 575–581, 586, 642, 677–681, 683, 696, 723, 726–727, 729–730, 732, 743–747, 751, 755, 757–758, 777, 780, 784–785, 790, 793–794, 809, 813, 815–816, 843–844, 846, 849, 854–855, 858–859, 861, 863–867, 869–870, 873–880, 882–892, 894, 896–897, 908–910, 912–914, 916–918, 920–922, 924, 927–933, 935–936, 938–939, 941, 952–954, 956–957, 959–961, 963, 975–978, 983, 1047, 1049–1054, 1056, 1061–1065, 1071–1074, 1076–1081, 1085–1090, 1097, 1134, 1136, 1141, 1152, 1161–1164, 1168, 1170–1172, 1176–1177, 1179–1186, 1188, 1190, 1193–1200, 1203–1207, 1210–1215, 1222, 1224, 1276, 1293–1296, 1320, 1342, 1372, 1377, 1380, 1383–1384, 1389, 1392, 1397, 1400, 1443–1447, 1454–1455, 1457, 1466–1467, 1469, 1486, 1499–1501, 1504, 1515, 1520, 1525, 1554–1556, 1561–1562, 1564–1565, 1567–1568, 1570–1589, 1591–1593, 1595–1606, 1608, 1625–1627, 1629–1631, 1633–1635, 1640, 1645–1647, 1652, 1679, 1710–1736, 1741–1742, 1744–1746, 1749–1750, 1753–1754, 1757–1758, 1777–1778, 1787–1788, 1798–1799, 1806–1807, 1813–1816, 1822–1825, 1831–1832, 1838–1839, 1859–1860, 1876–1882, 1884, 1892–1893, 1931, 1933–1935, 1937, 1939, 1942, 1944–1957, 1959–1960, 1969–1971, 1983–1984, 1993–1994, 1996, 1998–2000, 2007–2009, 2018–2019, 2021–2023, 2031–2032, 2043, 2045–2046, 2048, 2050–2053, 2055–2057, 2060, 2062, 2069–2070, 2077–2078, 2088–2089, 2099–2102, 2109–2112, 2123
rhsmlib/services
   register.py1152082%31, 69, 82–83, 85–86, 88–89, 91–92, 125, 180, 215, 246, 248, 260, 269, 288, 295, 297
TOTAL17432446674% 

Tests Skipped Failures Errors Time
2416 14 💤 0 ❌ 0 🔥 32.296s ⏱️

@jirihnidek
Copy link
Contributor Author

jirihnidek commented Dec 9, 2024

If you want to test this new functionality, then you have to create some testing environment using e.g. this REST API (replace localhost with your testing candlepin server):

curl --stderr /dev/null --insecure --user admin:admin --request POST \
    --data '{"id": "env-id-1", "name": "env-name-1", "description": "Testing env #1", "type": "foo"}' \
    --header 'accept: application/json' --header 'content-type: application/json' \
    https://localhost:8443/candlepin/owners/donaldduck/environments

Testing script (needs library.sh from https://github.com/jirihnidek/rhsm-dbus-examples/):

#!/bin/bash

source ./library.sh

export username="admin"
export password="admin"
export org_id="donaldduck"

start_register_server

# Try to register using username and password
echo "Registering using org, username and password..."
dbus-send --address=${my_addr} --print-reply --dest='com.redhat.RHSM1.Register' \
        '/com/redhat/RHSM1/Register' \
        com.redhat.RHSM1.Register.Register \
        string:"${org_id}" \
        string:"${username}" \
        string:"${password}" \
        dict:string:string:"environment_names","env-name-1","environment_type","foo" \
        dict:string:string:"","" \
        string:"" | gawk '/string/{ $1 = ""; print $0; }' | sed 's/\"//' | sed 's/\(.*\)\"/\1/' > /root/register_output.json

print_registration_result $?

stop_register_server

@rverdile
Copy link
Contributor

I was playing around with this through rhc, it seems to do everything we need it to! Any reason to keep this in draft?

@jirihnidek
Copy link
Contributor Author

I was playing around with this through rhc, it seems to do everything we need it to! Any reason to keep this in draft?

I would like to add more unit tests and maybe polish commit little bit. After that, I will convert it to PR and it will be ready for review.

@jirihnidek jirihnidek force-pushed the jhnidek/reg_env_names branch from 8a9bbeb to 7092ac7 Compare December 13, 2024 16:56
@jirihnidek jirihnidek changed the title feat: Enable register with environment names feat: Enable register with environment names & type Dec 13, 2024
@jirihnidek jirihnidek force-pushed the jhnidek/reg_env_names branch from 7092ac7 to 855f9ec Compare December 13, 2024 17:44
@jirihnidek jirihnidek marked this pull request as ready for review December 13, 2024 17:52
@jirihnidek
Copy link
Contributor Author

@rverdile The PR is ready for review.

Copy link
Contributor

@m-horky m-horky left a comment

Choose a reason for hiding this comment

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

The patch behaves as it should. The code looks fine, from birds-eye point of view.

@rverdile
Copy link
Contributor

tried with rhc and looks good!

Copy link
Contributor

@jvlcek jvlcek left a comment

Choose a reason for hiding this comment

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

This looks good @jirihnidek. I have a couple small suggestions. I apologies if I my assumptions are incorrect. Please let me know if they are.

I am not easily able to test this with the condleping-container because I am on an arm64 and the container expects amd64. @m-horky had reported The patch behaves as it should. With him running it and me looking over the code I think we should be good once you respond to my feedback.

Thank you. Joe

src/rhsmlib/services/register.py Show resolved Hide resolved
src/rhsmlib/services/register.py Outdated Show resolved Hide resolved
src/rhsmlib/services/register.py Outdated Show resolved Hide resolved
src/rhsmlib/services/register.py Outdated Show resolved Hide resolved
test/rhsmlib/services/test_register.py Show resolved Hide resolved
@jirihnidek jirihnidek force-pushed the jhnidek/reg_env_names branch 2 times, most recently from 9096534 to 5ba01cf Compare December 19, 2024 16:03
* Allow to register also with environment names. It is possible
  to use environment names with username & password and
  activation-keys & organization authentication
* It is not allowed to use `environmets` and `environment_names`
  registration options together, because it is not possible to use
  env. IDs and names together on candlepin server
* Introduce environment_type as another registration_option
  * When consumer object is returned and it contains environments,
    then all environments are checked if type of environment
    matches given environment_type. If type is missing or is
    different, then system is unregistered and exception is
    raised
* Modified few unit tests related to environments
* Added few unit tests
@jirihnidek jirihnidek force-pushed the jhnidek/reg_env_names branch from 5ba01cf to 8140ecc Compare December 19, 2024 17:39
@jirihnidek
Copy link
Contributor Author

@jvlcek The PR should be ready for review again.

@jvlcek jvlcek requested review from jvlcek and removed request for jvlcek December 19, 2024 18:14
@jvlcek
Copy link
Contributor

jvlcek commented Dec 19, 2024

@jirihnidek Thank you for addressing all of my comments. I approve this PR now. However it appears I may not be authorized to officially mark this as approved. Regardless you have my approval to have this merged.

@jirihnidek jirihnidek merged commit 76d9858 into main Dec 19, 2024
24 checks passed
@jirihnidek jirihnidek deleted the jhnidek/reg_env_names branch December 19, 2024 21:34
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.

4 participants