Skip to content

CSI-2625 add CSI IBM Driver operator CI#189

Draft
matancarmeli7 wants to merge 72 commits intodevelopfrom
task/CSI-2625_add_another_stage_before_the_merge
Draft

CSI-2625 add CSI IBM Driver operator CI#189
matancarmeli7 wants to merge 72 commits intodevelopfrom
task/CSI-2625_add_another_stage_before_the_merge

Conversation

@matancarmeli7
Copy link
Contributor

No description provided.

@matancarmeli7 matancarmeli7 changed the title add CSI IBM Driver operator CI CSI-2625 add CSI IBM Driver operator CI Jul 1, 2021
zingero
zingero previously approved these changes Jul 5, 2021
Signed-off-by: matancarmeli7 <matan.carmeli7@gmail.com>
Signed-off-by: matancarmeli7 <matan.carmeli7@gmail.com>
Signed-off-by: matancarmeli7 <matan.carmeli7@gmail.com>
Signed-off-by: matancarmeli7 <matan.carmeli7@gmail.com>
Comment on lines 12 to 16
Copy link
Contributor

Choose a reason for hiding this comment

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

consider using a shorter command with /dev/null and just checking the exit status

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@oriyarde oriyarde Oct 26, 2021

Choose a reason for hiding this comment

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

sorry for the confusion, I think the status check should remain inside the function, to keep it "boolean" as before.
I just wanted to avoid keeping the entire command output in a variable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you mean move those lines into is_private_branch_component_image_exists function?

Copy link
Contributor

Choose a reason for hiding this comment

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

no, I mean restore the function to how it was before (echoing true/false), but according to $? == "0", so the caller would continue to compare using == "true"

Copy link
Contributor

Choose a reason for hiding this comment

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

hope it's clear

Signed-off-by: matancarmeli7 <matan.carmeli7@gmail.com>
Signed-off-by: matancarmeli7 <matan.carmeli7@gmail.com>
Signed-off-by: matancarmeli7 <matan.carmeli7@gmail.com>
Signed-off-by: matancarmeli7 <matan.carmeli7@gmail.com>
Signed-off-by: matancarmeli7 <matan.carmeli7@gmail.com>
driver_component=$1
image_to_check=$csiblock_docker_registry_username/ibm-block-csi-$driver_component:$branch_image_tag
is_image_tag_exists=false
export driver_image_inspect=$(docker manifest inspect $image_to_check &> /dev/null; echo $?)
Copy link
Contributor

Choose a reason for hiding this comment

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

where is driver_image_inspect used?
why does it sound like an action? if I understand correctly, it is not a function

- name: Login to DockerHub
uses: docker/login-action@v1
with:
username: '${{ secrets.CSIBLOCK_DOCKER_REGISTRY_USERNAME }}'
Copy link
Contributor

Choose a reason for hiding this comment

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

I would avoid using the current registry/user name (csiblock) in our variables, since it can always change in the future

- name: Deploy ibm block csi operator
run: |
build/ci/github_actions/operator/deploy_operator.sh
- name: Deploy ibm block csi driver
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this under create_cluster?

Copy link
Contributor

@oriyarde oriyarde left a comment

Choose a reason for hiding this comment

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

same as here

@oriyarde oriyarde marked this pull request as draft February 7, 2022 14:30
@matancarmeli7 matancarmeli7 marked this pull request as ready for review February 21, 2022 08:29
Signed-off-by: matancarmeli7 <matan.carmeli7@gmail.com>
@oriyarde oriyarde marked this pull request as draft March 6, 2022 11:23
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

Comments