Skip to content

CSI-2625 add CSI IBM Driver operator CI #189

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

Draft
wants to merge 72 commits into
base: develop
Choose a base branch
from

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 <[email protected]>
Comment on lines 12 to 16
export driver_image_inspect=`docker manifest inspect $csiblock_docker_registry_username/ibm-block-csi-$driver_component:$target_specific_tag`
if [[ "$driver_image_inspect" != "" ]]; then
is_image_tag_exists=true
fi
echo $is_image_tag_exists
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 <[email protected]>
Signed-off-by: matancarmeli7 <[email protected]>
Signed-off-by: matancarmeli7 <[email protected]>
Signed-off-by: matancarmeli7 <[email protected]>
Signed-off-by: matancarmeli7 <[email protected]>
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
@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