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

test: fix unset variable in preconfig integration test #542

Merged
merged 1 commit into from Feb 14, 2022

Conversation

annekebr
Copy link
Collaborator

@annekebr annekebr commented Feb 11, 2022

As the actual update of the config values happens in its own bash function, the IPP variable is no longer set when performing the values.yaml update resulting in an empty value in the yaml file (imagePullPolicy: "").

Fixes #

Description

Checklist

  • PR is rebased to/aimed at branch develop
  • PR follows Contributing Guide
  • Added tests (if necessary)
  • Extended README/Documentation (if necessary)
  • Adjusted versions of image and Helm chart in values.yaml and Chart.yaml (if necessary)

@codecov-commenter
Copy link

Codecov Report

Merging #542 (c85c4bb) into develop (29803af) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #542   +/-   ##
========================================
  Coverage    94.40%   94.40%           
========================================
  Files           22       22           
  Lines         1125     1125           
========================================
  Hits          1062     1062           
  Misses          63       63           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 29803af...c85c4bb. Read the comment docs.

Copy link
Collaborator

@xopham xopham left a comment

Choose a reason for hiding this comment

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

happy to approve this. however, does it actually cause an error?
if so, please update the description...

@annekebr
Copy link
Collaborator Author

annekebr commented Feb 11, 2022

@xopham I'm pretty sure that it was intended to actually replace the value for the imagePullPolicy in https://github.com/sse-secure-systems/connaisseur/blob/master/tests/integration/integration-test.sh#L286 by the value IPP that seems to be set only for this purpose, but as the actual update is invoked in its own function the variable is no longer set. I was wondering why the test still works as Kubernetes says that it only allows Never, IfNotPresent and Always as imagePullPolicy according to their docs, but in my local tests an empty string seems to be valid as well. Didn't allow that in #458 though, that's why I stumbled across it. I updated #458 accordingly, but I thought that this might be worth fixing anyway before it actually causes issues.

@xopham
Copy link
Collaborator

xopham commented Feb 13, 2022

Thx for updating 🙏
Might that have caused #497 ?

Copy link
Collaborator

@xopham xopham left a comment

Choose a reason for hiding this comment

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

Lgtm

@annekebr
Copy link
Collaborator Author

It seems that the issue was introduced in #497, but as imagePullPolicy="" defaults to Always it still worked as intended :)

@annekebr annekebr merged commit c85c4bb into develop Feb 14, 2022
@xopham xopham deleted the test/fix-preconfig-integration-test branch February 21, 2022 09:24
@phbelitz phbelitz mentioned this pull request Mar 2, 2022
6 tasks
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