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

update byoc doc #116

Merged
merged 4 commits into from Jan 21, 2022
Merged

Conversation

xytian315
Copy link
Contributor

@xytian315 xytian315 commented Dec 11, 2021

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:

/kind bug
/kind cleanup
/kind deprecation
/kind design

/kind documentation

/kind feature
/kind release

What this PR does / why we need it:
update byoc doc
Which issue(s) this PR fixes:

Fixes #102

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., usage docs, etc.:


@phenixblue phenixblue added the documentation Improvements or additions to documentation label Jan 10, 2022
@phenixblue
Copy link
Contributor

I've been away on Holliday and am just returning. I will still be slow to respond as I get caught up with things, but hope to get this reviewed soon.

@phenixblue
Copy link
Contributor

One quick note, please setup commit signing for your contributions. I just realized we have that enforcing, but don't have it documented in the CONTRIBUTING.md file (my apologies for any confusion). I opened #117 to track the addition of that info to the Contributing Docs.

More info on how to setup commit signing can be found here:

https://docs.github.com/en/authentication/managing-commit-signature-verification/displaying-verification-statuses-for-all-of-your-commits

@xytian315 xytian315 requested a review from a team as a code owner January 11, 2022 06:04
@xytian315
Copy link
Contributor Author

One quick note, please setup commit signing for your contributions. I just realized we have that enforcing, but don't have it documented in the CONTRIBUTING.md file (my apologies for any confusion). I opened #117 to track the addition of that info to the Contributing Docs.

More info on how to setup commit signing can be found here:

https://docs.github.com/en/authentication/managing-commit-signature-verification/displaying-verification-statuses-for-all-of-your-commits

Done! Thanks so much!

Copy link
Contributor

@phenixblue phenixblue left a comment

Choose a reason for hiding this comment

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

A couple of small things.

docs/install.md Outdated Show resolved Hide resolved
docs/install.md Outdated Show resolved Hide resolved
@xytian315
Copy link
Contributor Author

xytian315 commented Jan 14, 2022

there is one problem I noticed that, in the init code, when the secret exists, we were "Waiting for race winning pod to startup" by checking magtape/updated-by-pod label, and this label will not be added if magtape_tls_byoc, so when magtape_tls_byoc, there will be an error since no such label exist magtape/updated-by-pod. I added a commit a7f7bb7, just add a condition
if not magtape_tls_byoc: to the existing logic of checking magtape/updated-by-pod label.

I tested locally, the init container started successfully.

@xytian315
Copy link
Contributor Author

FYI: another small issue I ran with is that the black version I installed locally wasn't exactly the same as what we are specifying in the CI job. So previously when I run make ci-lint-python, there were a lot of changes in other files, which I didn't check in earlier.

I noticed from last CI job here https://github.com/tmobile/magtape/runs/4892244656?check_suite_focus=true, there was a specific black version, I changed them locally accordingly and create a new commit to fix the issue 3c8ef3d
Hopefully this fixed the CI job 🤞

Maybe we can change the contribution guide to specify the black version we are using in CI job.

Copy link
Contributor

@phenixblue phenixblue left a comment

Choose a reason for hiding this comment

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

False positive on the codeQL check

/lgtm

@phenixblue phenixblue merged commit b751766 into tmobile:master Jan 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix Bring Your Own Cert Docs
2 participants