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

Task/csi 3503 add verification #224

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

Conversation

matancarmeli7
Copy link
Contributor

No description provided.

Signed-off-by: matancarmeli7 <[email protected]>
Signed-off-by: matancarmeli7 <[email protected]>
Signed-off-by: matancarmeli7 <[email protected]>
app.kubernetes.io/instance: ibm-block-csi-operator
app.kubernetes.io/managed-by: ibm-block-csi-operator
product: ibm-block-csi-driver
app.kubernetes.io/name: ibm-block-csi-operator
Copy link
Contributor

Choose a reason for hiding this comment

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

most of the diff looks like "rearranging lines", can we avoid it so the diff will be minimal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I needed to do it since our yamls weren't aligned and that the CI won't fail, it will be easier for you if I will separate the PR's? one PR will be the yamls diffs and the other one will be the scripts that check it

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, that would be great!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opened two PRs, this is how you wanted it?
#227
#226

ARG YQ_VERSION=v4.13.5
ARG ARCH=yq_linux_386
RUN ARCH=$(if [ "$(uname -m)" = "x86_64" ]; then echo "amd64"; else echo "$(uname -m)" ; fi) \
&& wget https://github.com/mikefarah/yq/releases/download/${YQ_VERSION}/yq_linux_${ARCH} -O /usr/bin/yq \
Copy link
Contributor

Choose a reason for hiding this comment

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

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 here

Copy link
Contributor

@roysahar roysahar left a comment

Choose a reason for hiding this comment

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

verify_no_roles_diff (){
echo "check roles alignment"
source hack/get_information_helper.sh
are_manifest_files_exsists_in_current_csi_version
Copy link
Contributor

Choose a reason for hiding this comment

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

please:

  1. address all conversations from PRs with common code
  2. sync the changes between said PRs so that their common diff is aligned
  3. change all but one said PRs to "draft", so that we stop doing double work
  4. when you want to change back a PR from "draft", ensure it is up to date with develop first

@matancarmeli7
Copy link
Contributor Author

fyi #224 (comment)

@matancarmeli7 matancarmeli7 marked this pull request as draft December 1, 2021 13:53
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