OCPBUGS-38450: Fix copy artifacts for all CPU architectures #1855
Conversation
openshift-ci-robot
added
jira/severity-important
|
@rwsu: This pull request references Jira Issue OCPBUGS-38450, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
the
approved
rwsu
changed the title
|
/retest-required |
|
/lgtm |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@rwsu: This pull request references Jira Issue OCPBUGS-38450, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/retest-required |
|
/hold Revision 654d87c was retested 3 times: holding |
openshift-ci
bot
added
do-not-merge/hold
openshift-merge-robot
added
the
needs-rebase
Currently it expects the filename to be node.x86_64.iso which doesn't work for non X86 architectures. Updated RsyncOptions to copy any iso file to include files for any CPU architecture. Authored by Andrea Fasano <afasano@redhat.com>
openshift-merge-robot
removed
the
needs-rebase
|
/retest-required |
|
/test e2e-aws-ovn-serial |
| if o.OutputName == "" { | ||
| return nil | ||
| } | ||
| // AssetDir doesn't exist in unit test fake filesystem |
There was a problem hiding this comment.
| // AssetDir doesn't exist in unit test fake filesystem | |
| // AssetDir doesn't exist in unit test fake filesystem |
There was a problem hiding this comment.
I don't see a difference here.
There was a problem hiding this comment.
My suggestion was to remove the comment, given that it's related to a test
| newPath := filepath.Join(filepath.Dir(path), o.OutputName) | ||
|
|
||
| // Check if another file has the same name | ||
| if _, err := os.Stat(newPath); err == nil { |
There was a problem hiding this comment.
Since newPath is invariant, couldn't be moved out this check before starting looking in the folder?
There was a problem hiding this comment.
That would assume the ISO isn't written to a subdirectory under o.AssetDir. Which is the case now. But by not assuming it is invariant, makes it less coupled to the node-joiner implementation.
| } | ||
| } | ||
|
|
||
| err = filepath.Walk(o.AssetsDir, func(path string, info os.FileInfo, err error) error { |
There was a problem hiding this comment.
wondering if in this case using os.ReadDir could lead to a simpler implementation
There was a problem hiding this comment.
I tried os.ReadDir. It is pretty much the same.
Updated unit-test to change the assertion on output filename to assertion on the destination path. Previously the filename was asserted against the RSyncOptions's Destination field which contained the target filename and path. Now the Destination field is a directory path. Because fs.FS doesn't support write operations we cannot fake writing or renaming the output file, which also blocks asserting the final output name and path.
|
/retest-required |
1 similar comment
|
/retest-required |
|
@rwsu: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| if err != nil { | ||
| if os.IsNotExist(err) { | ||
| return nil | ||
| } else { |
There was a problem hiding this comment.
For code readability, I'd prefer to use more happy path here
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andfasano, pawanpinjarkar, rwsu The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/unhold |
openshift-ci
bot
removed
the
do-not-merge/hold
The "oc adm node-image create" command currently expects the filename to be node.x86_64.iso which doesn't work for non x86_64 architectures. Updated RsyncOptions to copy any iso file to include files for any CPU architecture.
Authored by @andfasano