task CSI-2892/add community checks to operator#157
Conversation
matancarmeli7
requested review from
ArbelNathan,
annaparienti,
barakda,
oriyarde,
roysahar-ibm and
zingero
oriyarde
left a comment
oriyarde
left a comment
There was a problem hiding this comment.
don't forget to add the ticket number in the PR title
matancarmeli7
changed the title
| gh pr checks $community_operators_branch --repo $forked_community_operators_repository | ||
| } | ||
|
|
||
| wait_fot_checks_to_start(){ |
There was a problem hiding this comment.
why is there no for in the filename?
There was a problem hiding this comment.
the filename wasn't changed
|
|
||
| wait_fot_checks_to_start(){ | ||
| community_operators_branch=$1 | ||
| forked_community_operators_repository=$2 |
There was a problem hiding this comment.
is this necessary for gh_pr_checks_command?
if so, how come community_operators_branch is passed but not this one?
There was a problem hiding this comment.
removed it,
forked_community_operators_repository is an environment variable that I set in the CI
There was a problem hiding this comment.
if both are passed, then I think their name inside the gh_pr_checks_command function should be more general, e.g. branch and repository
| echo $github_token > github_token.txt | ||
| gh auth login --with-token < github_token.txt | ||
| gh repo fork operator-framework/community-operators --clone community-operators-fork | ||
| cd community-operators-fork | ||
| git remote set-url origin https://csiblock:$github_token@github.com/csiblock/community-operators.git | ||
| git fetch upstream | ||
| git rebase upstream/master | ||
| git push origin master --force |
There was a problem hiding this comment.
could this also be a function? with a name about whatever it does? something like fork_and_clone?
There was a problem hiding this comment.
update seems pretty general to me.
would something like sync_fork(_master_branch)?_from_upstream be more accurate?
|
|
||
| export passed_k8s_checks=true | ||
| export passed_openshift_checks=true | ||
| # CSI-3172 - run rad hat bot checks |
There was a problem hiding this comment.
| # CSI-3172 - run rad hat bot checks | |
| # CSI-3172 - run Red Hat bot checks |
There was a problem hiding this comment.
rad hat would have been a much cooler name for them
| #!/bin/bash -xe | ||
| set +o pipefail | ||
|
|
||
| gh_pr_checks_command (){ |
There was a problem hiding this comment.
if we run the command, I suggest naming the function accordingly:
| gh_pr_checks_command (){ | |
| run_gh_pr_checks_command (){ |
There was a problem hiding this comment.
isn't gh_pr_checks enough?
There was a problem hiding this comment.
From this article:
In general, use nouns for classes, verbs for functions, and names that show purpose for variables, attributes and arguments. Avoid (Systems) Hungarian notation in dynamically typed languages since data types can change. Use lower Camel Case for variables and methods. Use Pascal Case for classes and their constructors. Use Screaming Case for constants.
Thus, I suggest using a verb.
There was a problem hiding this comment.
I agree with the article, though I think in this case, the function is being used more like a command alias than a function.
I think this distinction is important, otherwise we might wrap all the commands we use with a long, low-level-detail named functions of the form run_X_command.
if you still disagree, at least consider the shorter get_pr_checks
There was a problem hiding this comment.
get_pr_checks sounds great.
If you want to continue with the discussion about naming, I suggest having a session with Arbel.
| } | ||
| wait_for_checks_to_complete(){ |
There was a problem hiding this comment.
do we want to have a single line break between functions? what is the convention here?
please try to use an IDE that helps with such conventions
| repo_pr=`gh pr list --repo $forked_repository | grep $community_operators_branch` | ||
| if [[ "$repo_pr" == *"$community_operators_branch"* ]]; then |
There was a problem hiding this comment.
could you give an example for an output of the gh pr list command that would result in the if condition being false?
There was a problem hiding this comment.
and what does it mean if there is no pr with the required branch? shouldn't it fail?
There was a problem hiding this comment.
in addition, I see this code in at least 3 different places in this PR.
please move it to a common location
| #!/bin/bash -xel | ||
| set +o pipefail | ||
|
|
||
| edit_operator_image_in_csv_yaml_file (){ |
There was a problem hiding this comment.
IMO no need for _file, since it's implied by the yaml
| #!/bin/bash -xe | ||
| set +o pipefail | ||
|
|
||
| echo $'yq() {\n docker run --rm -e operator_image_for_test=$operator_image_for_test -i -v "${PWD}":/workdir mikefarah/yq:4 "$@"\n}' >> /home/runner/.bash_profile |
There was a problem hiding this comment.
please apply conversations from other PRs where applicable
| env: | ||
| original_community_operators_repository: "k8s-operatorhub/community-operators" | ||
| original_community_operators_repository_prod: "redhat-openshift-ecosystem/community-operators-prod" | ||
| forked_community_operators_repository: "csiblock/community-operators" |
There was a problem hiding this comment.
please change the forked terminology to fork wherever applicable
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>
|
|
||
| if [ $passed_k8s_checks == "false" ] || [ $passed_openshift_checks == "false" ] | ||
| then | ||
| echo "some test failed :(" |
There was a problem hiding this comment.
it's not clear which test failed (nor if more than 1 failed)
| image_version=`cat version/version.go | grep -i driverversion | awk -F = '{print $2}'` | ||
| image_version=`echo ${image_version//\"}` | ||
|
|
||
| echo "::set-output name=image_branch_tag::${image_version}" No newline at end of file |
oriyarde
left a comment
oriyarde
left a comment
There was a problem hiding this comment.
if this PR is not in plan to be merged in the near future, please change it to "draft"
3 participants
No description provided.