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
Onboarding CLI | Flux bootstrapping and git authentication credentials #3578
Onboarding CLI | Flux bootstrapping and git authentication credentials #3578
Changes from 18 commits
b8425e6
7e4fd03
118c73e
006f45a
da28aa8
80667a4
65dfcf2
0b27797
5e4d52b
7faee99
e3cc8a9
f58abae
b6f1064
51355a3
49d284a
03879c0
1395ede
c9b4477
c3dfb5c
0059abb
0ed19ee
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.
not sure if yes as default is a safe choice. let review in the standup the list of questions and understand the side effects of it.
it also feel an important decision to record as ADR
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.
maybe we could add a hint that's this option is currently for testing only
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.
Looking into the questions of
confirmyes
we have:suggestion would be to change semantics for this flag from
always yes
todefault option if not introduced input
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.
follow up: change the silent semantics to use default behaviour where the default behaviour for each step should be the most conservative one: dont do a state change or mutation
for now:
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.
should be token or password? i could see https://fluxcd.io/flux/cmd/flux_bootstrap_git/ is password
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.
yeah in flux is password but it's very confusing as adding the git https basic auth with password doesn't work anymore and only accept token
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.
yes but i think that flux gets it from http where basic auth is username and password https://en.wikipedia.org/wiki/Basic_access_authentication
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 auth with username and password is no longer accepted https://docs.github.com/en/rest/overview/authenticating-to-the-rest-api?apiVersion=2022-11-28#authenticating-with-username-and-password
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.
ahh we add
--token-auth
to flux configuration so that it can handle it as a tokenThere 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 guess flux is correct calling it password
https://fluxcd.io/flux/components/source/gitrepositories/#basic-access-authentication
as it is how is defined in the standard https://datatracker.ietf.org/doc/html/rfc7617#section-2
we could review this in the catchup too
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 wonder if this is failure cause, if we dont have as a requirement that flux is bootstrapped, the fact that flux is not bootstrapped is just an expected scenario that we handle instead of an error.
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.
eneko to test out
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.