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

ci: Check if RBAC rules are in sync #319

Merged
merged 2 commits into from
Jul 11, 2018
Merged

ci: Check if RBAC rules are in sync #319

merged 2 commits into from
Jul 11, 2018

Conversation

krnowak
Copy link
Contributor

@krnowak krnowak commented Jul 11, 2018

Signed-off-by: Krzesimir Nowak krzesimir@kinvolk.io

Fixes #250.

@krnowak krnowak force-pushed the krnowak/rbac-check branch 3 times, most recently from ab38258 to 2dd829c Compare July 11, 2018 12:16
@krnowak
Copy link
Contributor Author

krnowak commented Jul 11, 2018

The failure will go away when #317 is merged.

@krnowak krnowak force-pushed the krnowak/rbac-check branch 2 times, most recently from 00459f5 to 4fef5a0 Compare July 11, 2018 12:23
Copy link
Contributor

@indradhanush indradhanush left a comment

Choose a reason for hiding this comment

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

LGTM mostly. Have some questions that rise out of my lack of sound understanding of bash


set -euo pipefail

dir=$(dirname "${BASH_SOURCE[0]}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: Marking this as readonly may be a good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, probably. Will do.

do
# strip the newline
line="${line%\\n}"
if [[ -z "${line}" ]]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why have this check at all if you don't want to do anything here? 🤔

Copy link
Contributor Author

@krnowak krnowak Jul 11, 2018

Choose a reason for hiding this comment

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

That prevents the code from other branches in the if else cascade to be executed. Did it that way to avoid adding yet another indentation level. There are several ways of doing that:

if [[ -z "${line}" ]]; then
  :
elsif …
then
  …
fi

or

if [[ -n "${line}" ]]; then # note that it is -n, not -z
  if …
  then
    …
  fi
fi

or

if [[ -z "${line}" ]]; then
  continue # will work only if we are in a loop
fi
if …
then
  …
fi

test_rules="$(read_rules "${dir}/../${test_path}")"

# rely on transitivity, if example == chart and chart == test then
# example == test
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wanted to say that I like this. 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was reading "Elements of programming" and "From mathematics to generic programming" so I guess they bent my brain a bit.

then
# rules begin from the next line
ignore=0
elif [[ "${line}" =~ '{{' ]]
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same answer here. :P

Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
Some of the steps are always executed or are executed on
failure. Those steps require some things to be set up. But if some
test fails before those things are set up, then we get spurious error
messages from these steps too. This may happen when RBAC rules are not
in sync, so the tests will fail before setting up the cluster and
pushing the docker image to container registry, so there will be no
cluster to get the logs from (even likely there won't be even a
kubectl binary to use) and to delete nor images to remove from the
registry.

Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
@krnowak
Copy link
Contributor Author

krnowak commented Jul 11, 2018

Updated and rebased on top of current master, so the test should pass now.

Copy link
Contributor

@indradhanush indradhanush left a comment

Choose a reason for hiding this comment

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

LGTM. 👍

@krnowak krnowak merged commit edee233 into master Jul 11, 2018
@krnowak krnowak deleted the krnowak/rbac-check branch July 11, 2018 15:46
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.

2 participants