fix: check status created a machine-id file #3965
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
m-horky
suggested changes
Dec 19, 2023
There was a problem hiding this comment.
This would solve it, but I am not really a fan of some of the parts.
- Have you considered changing the original
client.get_machine_idto accept acreateflag, instead of creating a new function with similar name in the utils?
insights-core/insights/client/client.py
Lines 307 to 308 in f547907
- If that would be too complicated, I found out you are only checking if the returned value is not None. Would it make sense to rename it to something like
machine_id_exists()returning a bool?
I considered that, but there is also a flag to create a new file, independently if it already exists. After this fix, I was considering to create a new ticket to make some effort refactoring this utility because is a bit entangled. Ideally, discerning the functions that use this method into 2, the ones that need by any cost an id, and the ones that doesn't need an id.
Not only, if it exists it returns the value of machine-id |
Changelog
|
m-horky
suggested changes
Dec 19, 2023
| @@ -42,10 +44,10 @@ def test_registration_check_ok_reg_then_unreg(get_proxies, _init_session, _): | |||
| assert conn.api_registration_check() == '2019-04-10' | |||
|
|
|||
|
|
|||
| @patch("insights.client.connection.generate_machine_id", return_value='xxxxxx') | |||
| @patch("insights.client.connection.False", return_value=True) | |||
There was a problem hiding this comment.
This is probably a bad copy-paste
|
test me |
|
Verification pass on this open PR, please refer details: https://issues.redhat.com/browse/CCT-161 |
|
@xiangce This can be on the next egg release |
This method returns a boolean whether the machine-id file exists or not. This will be used to check the file before call, generate_machine_id to get the machine-id, as this last method will create a new id if it doesn't exist. Resolves: CCT-161 Signed-off-by: ahitacat <[email protected]>
When checking registration status, first check if the machine-id file exists. When checking status it is not needed to generate a machine-id file if it doesn't exists. Resolves: CCT-161 Signed-off-by: ahitacat <[email protected]>
Also create a new test to check a new machine id is created if the file does not exists. Signed-off-by: ahitacat <[email protected]>
xiangce
pushed a commit
that referenced
this pull request
Feb 8, 2024
* feat: add machine_id_exists method This method returns a boolean whether the machine-id file exists or not. This will be used to check the file before call, generate_machine_id to get the machine-id, as this last method will create a new id if it doesn't exist. Resolves: CCT-161 Signed-off-by: ahitacat <[email protected]> * fix: use machine_id_exists before checking host status When checking registration status, first check if the machine-id file exists. When checking status it is not needed to generate a machine-id file if it doesn't exists. Resolves: CCT-161 Signed-off-by: ahitacat <[email protected]> * fix: Change test that generated id and now use get_machineid Also create a new test to check a new machine id is created if the file does not exists. Signed-off-by: ahitacat <[email protected]> --------- Signed-off-by: ahitacat <[email protected]> (cherry picked from commit 5697ed4)
xiangce
added a commit
that referenced
this pull request
Feb 9, 2024
xiangce
added a commit
that referenced
this pull request
Feb 9, 2024
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
All Pull Requests:
Check all that apply:
Complete Description of Additions/Changes:
When insights-client checked the status created a machine-id file to make some calls to the console service. That created a residual machine file if it had not access to the service (i.e. system was not registered to rhsm).
This PR refactors a bit the logic of generate_machine_id, and splits in a new function that will only get the machine id
get_machineid().Checking status and
generate_machine_idwill use this new function.Test have been changed and a new one has been created.
Resolves: CCT-161