-
Notifications
You must be signed in to change notification settings - Fork 45
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
ARTESCA-3572 Allow the removal of Metalk8s archives in iso-manager #3730
ARTESCA-3572 Allow the removal of Metalk8s archives in iso-manager #3730
Conversation
Hello sayf-eddine-scality,My role is to assist you with the merge of this Status report is not available. |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few comments (if you add salt module you also need to add unit tests 😉 )
salt/metalk8s/repo/configured.sls
Outdated
{% set removable_archives = salt.metalk8s.get_removable_archives() %} | ||
{% for env in removable_archives %} | ||
Remove config for env {{ env }}: | ||
file.absent: | ||
- name: {{ salt.file.join(repo.config.directory, '99-' ~ env ~ '-registry.inc') }} | ||
{% endfor %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, to me we manage the full repo.config.directory
so if we don't know a file in this directory we just remove it, since it's a bit complicated with solutions I suggest just checking all 99-metalk8s-*
we remove all files that are not expected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 what if the Solution is called metalk8s-solution-example
..? I'll go with a regex such as 99-metalk8s-[0-9].*
, should be enough.
Also, this state is a bit strange, we're not really managing all the includes, only adding one for the current version 😕... I'll leave it like that but still confusing IMHO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe solutions shouldn't have the "99-" prefix but another one, or yes just a simple regex
Also, this state is a bit strange, we're not really managing all the includes, only adding one for the current version confused... I'll leave it like that but still confusing IMHO
That is my point, to me, we may want to change this state so that it basically "computes" the full directory content
Branches have divergedThis pull request's source branch To avoid any integration risks, please re-synchronize them using one of the
Note: If you choose to rebase, you may have to ask me to rebuild |
75ee9af
to
8dd0298
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
|
e07fa84
to
d8503b1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM
e2a95a2
to
af930d8
Compare
This comment was marked as resolved.
This comment was marked as resolved.
af930d8
to
fb95ab9
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
|
This adds a `-r`/`--rm-archive` option to the script, and deprecates the `--archive` in favor of `--add-archive` (for disambiguation). It also slightly reworks our module extracting info from the `product.txt` file in archives, to ensure the contents are valid at this stage, instead of having error management in the `metalk8s.archives.mounted` state.
To make sure we can safely remove an old ISO, we do so in upgrade tests, prior to running the test suite on the upgraded cluster.
fb95ab9
to
7d5ece4
Compare
/bypass_author_approval |
In the queueThe changeset has received all authorizations and has been added to the The changeset will be merged in:
The following branches will NOT be impacted:
There is no action required on your side. You will be notified here once IMPORTANT Please do not attempt to modify this pull request.
If you need this pull request to be removed from the queue, please contact a The following options are set: bypass_author_approval |
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue ARTESCA-3572. Goodbye sayf-eddine-scality. |
Component:
Context:
Allow the removal of the iso
Summary:
Acceptance criteria:
Closes: #ISSUE_NUMBER