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

Use Existing Secret in litmus-agent values.yaml #313

Closed
wants to merge 13 commits into from

Conversation

flockoftanks
Copy link

@flockoftanks flockoftanks commented Feb 27, 2023

What this PR does / why we need it:

The existing chart may force a user to put a password in source-control. This encourages bad security practices. This fix allows a user to specify a custom secret that can be used instead of the default template.

Which issue this PR fixes

(optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged)

Special notes for your reviewer:

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • DCO signed
  • Chart Version bumped
  • Variables are documented in the README.md

@Jasstkn
Copy link
Member

Jasstkn commented Feb 28, 2023

@flockoftanks Hi. Thanks for your PR. Please, take a look into our Contributing guide.

From what I can observe you need to do the following:

  • generate readme documentation with helm-docs (I also left a comment about format)
  • check if default configuration fails to install

charts/litmus-agent/values.yaml Outdated Show resolved Hide resolved
@flockoftanks
Copy link
Author

@flockoftanks Hi. Thanks for your PR. Please, take a look into our Contributing guide.

From what I can observe you need to do the following:

  • generate readme documentation with helm-docs (I also left a comment about format)
  • check if default configuration fails to install

Any idea what's causing the install to fail? I'm not familiar enough with litmus-agent, but I don't see any useful error messages in the log.

@Jasstkn
Copy link
Member

Jasstkn commented Feb 28, 2023

@ispeakc0de hi. can you help here? it gives the same error when I install chart with default values locally.

k logs -f install-litmus-agent-8cmfr
invalid character '<' looking for beginning of value

@Jasstkn
Copy link
Member

Jasstkn commented Mar 1, 2023

@ispeakc0de hi. can you help here? it gives the same error when I install chart with default values locally.

k logs -f install-litmus-agent-8cmfr
invalid character '<' looking for beginning of value

I think I may have an idea - this chart shouldn't be deployed during our CI. I will fix within the next few days.

@Shashankreddy6
Copy link

@ispeakc0de hi. can you help here? it gives the same error when I install chart with default values locally.

k logs -f install-litmus-agent-8cmfr
invalid character '<' looking for beginning of value

I think I may have an idea - this chart shouldn't be deployed during our CI. I will fix within the next few days.

Is this with respect to the added parameter of external secret in litmus-agent?

@Jasstkn
Copy link
Member

Jasstkn commented Mar 2, 2023

@ispeakc0de hi. can you help here? it gives the same error when I install chart with default values locally.

k logs -f install-litmus-agent-8cmfr
invalid character '<' looking for beginning of value

I think I may have an idea - this chart shouldn't be deployed during our CI. I will fix within the next few days.

Is this with respect to the added parameter of external secret in litmus-agent?

nope. it's just with default values. I think that the problem here is that this chart requires litmus to be installed. Therefore it's not suitable for independent tests. I'm going to exclude it.

charts/litmus-agent/values.yaml Outdated Show resolved Hide resolved
@flockoftanks
Copy link
Author

So, to clarify, am I waiting for something to change in the master branch that I will merge into this to fix the build?

Thanks for your help so far everyone!

@Shashankreddy6
Copy link

Any update on this @Jasstkn @gdsoumya .

@Jasstkn
Copy link
Member

Jasstkn commented Mar 10, 2023

@flockoftanks @Shashankreddy6 hey! I submitted PR: #314, waiting for approval.

@Jasstkn
Copy link
Member

Jasstkn commented Mar 12, 2023

@flockoftanks I made a fix in the main branch. Please, squash your commits, sign it and we will be able to merge it. Thanks!

@flockoftanks
Copy link
Author

Hey, sorry. Been AWOL. I'll get this merged up.

flockoftanks and others added 9 commits April 21, 2023 13:19
Signed-off-by: flockoftanks <[email protected]>
Signed-off-by: Corey Smith <[email protected]>
Signed-off-by: Corey Smith <[email protected]>
Signed-off-by: flockoftanks <[email protected]>
Signed-off-by: Corey Smith <[email protected]>
Signed-off-by: Corey Smith <[email protected]>
Signed-off-by: flockoftanks <[email protected]>
Signed-off-by: Corey Smith <[email protected]>
Signed-off-by: Corey Smith <[email protected]>
Signed-off-by: flockoftanks <[email protected]>
Signed-off-by: Corey Smith <[email protected]>
Signed-off-by: Corey Smith <[email protected]>
Signed-off-by: flockoftanks <[email protected]>
Signed-off-by: Corey Smith <[email protected]>
Signed-off-by: Corey Smith <[email protected]>
Signed-off-by: flockoftanks <[email protected]>
Signed-off-by: Corey Smith <[email protected]>
Signed-off-by: Corey Smith <[email protected]>
Signed-off-by: Corey Smith <[email protected]>
Signed-off-by: Corey Smith <[email protected]>
Signed-off-by: Corey Smith <[email protected]>
Signed-off-by: Corey Smith <[email protected]>
Signed-off-by: flockoftanks <[email protected]>
Signed-off-by: Corey Smith <[email protected]>
flockoftanks and others added 3 commits April 21, 2023 13:19
Signed-off-by: flockoftanks <[email protected]>
Signed-off-by: Corey Smith <[email protected]>
Bumps [helm/chart-testing-action](https://github.com/helm/chart-testing-action) from 2.3.1 to 2.4.0.
- [Release notes](https://github.com/helm/chart-testing-action/releases)
- [Commits](helm/chart-testing-action@v2.3.1...v2.4.0)

---
updated-dependencies:
- dependency-name: helm/chart-testing-action
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Corey Smith <[email protected]>
@flockoftanks
Copy link
Author

Not sure how to signoff on your commits that I synced to my branch. When I merged it said I didn't pass TCO checks because your most recent commits in my commit history weren't signed by me.

Following the directions in the "learn more" link and force pushing those signoffs, I think I got some of YOUR commits credit to me. Let me know if I need to change something.

@Calvinaud
Copy link
Contributor

Hello, I would to be interested by this PR (I can aslo work on it if needed). @flockoftanks Are you still working on this PR ?

@uditgaurav
Copy link
Member

Hi @flockoftanks , Feel free to close it and open a different one, If that makes things easier for you?

@sambonbonne
Copy link
Contributor

@uditgaurav maybe two weeks is a short delay but it seems the work has stopped on this PR (I'm not saying this negatively, we all have our own life). Would you accept a new PR from someone else? I may have some time for this next week but I don't want to supersede @flockoftanks work if that's not OK.

@sambonbonne
Copy link
Contributor

So, I hope this is not a problem but I opened a PR which reworks the current one and adds the ability to use an existing ConfigMap in addition to an existing Secret.

@Jonsy13
Copy link
Contributor

Jonsy13 commented Jul 16, 2024

Hi @flockoftanks Closing this PR due to inactivity and also there is one PR merged by @sambonbonne for same changes.
Feel free to open this PR again if there are any further changes.

@Jonsy13 Jonsy13 closed this Jul 16, 2024
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.

Use Existing Secret in litmus-agent values.yaml
8 participants