Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add Milestone Job integration tests for Helm Chart #171
Add Milestone Job integration tests for Helm Chart #171
Changes from all commits
beeabcc
4f0baa1
a971a60
e55fe1a
9e147b5
7444d4e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer we will work with one convention
Lower-case, with underscores to separate words. (most of our code is written in this way)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since I didn't write these I dont want to touch them too too much. It will become a huge PR. is that ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is already a huge PR. do you have an issue to fix it in another?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is in it's own commit so it is contained
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did we do changes for running with summon ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gives us the head of the repo because we pushd in and out of directories so paths get messed up.I was having a real tough time with paths and so this ensures that we are at the root of the repo (thanks @hughsaunders )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but what's happening in this line? you just declare it without doing anything with it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is nothing happening? Stop is being run
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See image attached
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didn't we change this to role? if not - why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is part of our previous tests (TEST_ID 1 - 16).
As we spoke about earlier, it is loaded in a different
.sh.yml
format and so any sort of refactoring will be a whole separate taskThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we have such a task?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those 'sh.yml' are for tests primarily so I think it is worth a discussion with the team. Since we are moving to BDD anyways it might be worth it to weigh this and see if we still need it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i may have asked this elsewhere but i can't find it - why do we need 2 of each resource? what is the purpose of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In short, we have some tests that test two secrets providers (
TEST_ID_18_helm_multiple_provider_multiple_secrets.sh
) and two secrets providers same secret (TEST_ID_19_helm_multiple_provider_same_secret.sh
) and two k8s secrets (TEST_ID_20_helm_service_account_does_not_exist.sh
)2 k8s resources cannot have the same role,rolebinding,serviceaccount,etc so we need to add two of everything for those ones
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why can't they have the same role and service account?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
asked this question to @eladkug and we agreed on two of everything
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd like to challenge that decision. why can't the secrets-providers have the same service account and role? isn't it closer to a customer's environment?
i also want to challenge the decision to have only one file for a secret that is used for 2 secrets. i think that having 2 files is more explicit to what you are trying to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eladkug please address Oren's first point
@orenbm I am not understanding why having a single files is an issue here. I prefer it being cleaner and having a single one that is dynamically updated and so that we don't have 2 places to clean up and two functions to create the secret etc. Earlier you mentioned why we have two of everything and you were right. I think where we can make resources reused and dynamically created we should because it will lead to a cleaner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we raised it before its different scenarios.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the multiple files issue is not a blocker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree