Skip to content
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

CSI-2625 create driver ci #344

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

Conversation

matancarmeli7
Copy link
Contributor

No description provided.

@matancarmeli7 matancarmeli7 changed the title create driver ci CSI-2625 create driver ci Jul 1, 2021
zingero
zingero previously approved these changes Jul 5, 2021
zingero
zingero previously approved these changes Jul 5, 2021
zingero
zingero previously approved these changes Jul 7, 2021
scripts/ci/deploy_driver.sh Outdated Show resolved Hide resolved
Signed-off-by: matancarmeli7 <matan.carmeli7@gmail.com>
Comment on lines 23 to 25
if [ "$docker_image_branch_tag" == "develop" ]; then
docker_image_branch_tag=latest
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can avoid double assignment if we move this if to the first assignment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what do you mean by "first assignment"?

Copy link
Contributor

Choose a reason for hiding this comment

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

i.e.

triggering_branch = ${CI_ACTION_REF_NAME}
.
.
.
if [ "$triggering_branch" == "develop" ]; then
    docker_image_branch_tag=latest
else
    docker_image_branch_tag=`echo $driver_image_tags | awk '{print$2}'`

Copy link
Contributor

Choose a reason for hiding this comment

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

but without backticks of course

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

Choose a reason for hiding this comment

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

could you also put the triggering_branch = ${CI_ACTION_REF_NAME} line before the driver_image_tags=$(scripts/ci/get_image_tags_from_branch.sh ... line?
it's hard to decipher the name github chose for this env var

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 Nov 29, 2021

Choose a reason for hiding this comment

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

also why is it sometimes docker_image... and not driver_image...?

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 +18 to +21
- name: Set up Python
uses: actions/setup-python@v2
with:
python-version: '3.9.7'
Copy link
Contributor

Choose a reason for hiding this comment

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

could you remind me what is this for?
it's hard to tell which setup is required by which logic

Copy link
Contributor

Choose a reason for hiding this comment

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

is it for docker/build-push-action?

cache-to: type=local,dest=/tmp/.buildx-new-${{ matrix.image_type }}
# Temp fix
# CSI-3164
# https://github.com/docker/build-push-action/issues/252
Copy link
Contributor

Choose a reason for hiding this comment

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

could you check if this is still needed?

steps:
- name: Checkout
uses: actions/checkout@v2
- name: CSI-controller- static code analysis
Copy link
Contributor

@oriyarde oriyarde Nov 28, 2021

Choose a reason for hiding this comment

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

Suggested change
- name: CSI-controller- static code analysis
- name: CSI-controller: static code analysis

(same for unit testing, etc...)
could you align the names to the ones in the jenkinsfile, as much as possible?
unless you know of an easy way to maintain them in one place

Copy link
Contributor

Choose a reason for hiding this comment

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

this would also mean:
k8s yamls validation should be:
K8s yamls validation (in both files)

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:31
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.

None yet

3 participants