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

BUG FIX: Make install-cni.sh check if the etcd cert file exist. #526

Merged
merged 2 commits into from
May 23, 2018

Conversation

bjhaid
Copy link
Contributor

@bjhaid bjhaid commented May 22, 2018

This will prevent it from terminating because stat returns non-zero
when called a non-existing file and also stat_output ends up being an
unbound variable.

Affected Component: install-cni container

@tmjd PTAL not sure if this needs a release note thanks!

Fix etcd cert file existence check in calico/cni

@CLAassistant
Copy link

CLAassistant commented May 22, 2018

CLA assistant check
All committers have signed the CLA.

@bjhaid bjhaid force-pushed the check_if_cert_file_exist branch from d418383 to 31bb1a6 Compare May 22, 2018 15:01
Copy link
Member

@tmjd tmjd left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this.
Could you clean up the first several lines of the while loop to be spaces too instead of tabs?

@@ -191,14 +191,16 @@ while [ "$should_sleep" == "true" ]; do
# version to the host. Just check the timestamp on the certificate to see if it
# has been updated. A bit hokey, but likely good enough.
if [ "$(ls ${SECRETS_MOUNT_DIR} 2>/dev/null)" ];
Copy link
Member

Choose a reason for hiding this comment

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

I think you should just change this line instead of adding a new check. I don't think there is a need for 2 checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed

@bjhaid
Copy link
Contributor Author

bjhaid commented May 22, 2018

@tmjd

Could you clean up the first several lines of the while loop to be spaces too instead of tabs?

The file has more places where it uses tabs and it's a mix of tabs and spaces for indentation should I just clean the entire file and make it use all spaces?

@bjhaid bjhaid force-pushed the check_if_cert_file_exist branch from 31bb1a6 to 22671c3 Compare May 22, 2018 15:50
This will prevent it from terminating because stat returns non-zero
when called a non-existing file and also stat_output ends up being an
unbound variable.

Affected Component: install-cni container.
@bjhaid bjhaid force-pushed the check_if_cert_file_exist branch from 22671c3 to dca283d Compare May 22, 2018 15:52
@bjhaid
Copy link
Contributor Author

bjhaid commented May 22, 2018

Added a second commit to consistently use spaces vs tabs

@tmjd
Copy link
Member

tmjd commented May 22, 2018

Yeah I'd be fine with cleaning up the whole file. I just wanted at least that loop to be consistent. Thanks.

not sure if this needs a release note

I don't think it is necessary.

# Kubernetes Secrets can be updated. If so, we need to install the updated
# version to the host. Just check the timestamp on the certificate to see if it
# has been updated. A bit hokey, but likely good enough.
if [ "$(ls ${SECRETS_MOUNT_DIR} 2>/dev/null)" ];
Copy link
Member

Choose a reason for hiding this comment

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

Your second commit reverted the changes in your first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

doh, fixed thanks!

@bjhaid bjhaid force-pushed the check_if_cert_file_exist branch from ec995fc to 1ddc24f Compare May 22, 2018 15:59
tmjd
tmjd previously approved these changes May 22, 2018
@tmjd tmjd dismissed their stale review May 22, 2018 17:38

Dismissing my review because someone else needs to review this first.

@tmjd
Copy link
Member

tmjd commented May 22, 2018

@caseydavenport WDYT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants