-
Notifications
You must be signed in to change notification settings - Fork 25
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
Add details for latest RU and code fixes for errors faced #79
Conversation
…ays the latest RU is installed
…le from GCS to BMS host if an older file with the same patch number (p6880880_<>.zip) exists in bms host
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.
Thanks for your work here; this PR seems to include many varied fixes, and I've tried to include comments form all of them. Splitting this into multiple independent PRs might help speed up the review cycle though.
Thanks for the review - the thought of creating multiple PRs crossed my mind, but I have changed these 4 files in a single branch during my testing in cloudVM (control node) and would need to cherry pick them into new branches (as PR is only possible on an entire branch and not commits within that branch). I will take this lesson forward into the future for sizeable PRs. |
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.
Almost almost there :-)
…link 3)comments in gcscopy.yml
…th file changes tested successfully
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.
Looking good! Just a few small stylistic things.
roles/patch/tasks/patch_vars.yml
Outdated
# limitations under the License. | ||
|
||
--- | ||
- name: Patch | include defaults |
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.
roles/patch/meta/main.yml includes a dependency on common
dependencies:
- { role: common }
So this step should be unnecessary
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.
Done
roles/patch/tasks/patch_vars.yml
Outdated
include_vars: | ||
file: roles/common/defaults/main.yml | ||
|
||
- name: Patch | Get the highest version number from gi_patches array |
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.
nit: patch should be lowercase
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.
Done
patch.yml
Outdated
- include_role: | ||
name: patch | ||
tasks_from: patch_vars.yml | ||
tags: populate_variables |
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.
Call it patch_vars for consistency maybe? I'll have a related comment inside patch_vars.yml
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.
Done
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.
LGTM :-)
Adding code fixes and JAN/APR 2021 RU details.
Will add in-line comments (in each file changed) next week for each of the files submitted for review with detailed error message that necessitated that particular code change and the successful testing of the fix.