-
Notifications
You must be signed in to change notification settings - Fork 171
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
570 external git server #646
Conversation
fdb3815
to
65ec2db
Compare
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.
Will continue PR review after we work through these comments--this is going to be a great option for larger deployments and lays the needed groundwork for external registry support 🥳
a3fb178
to
e6240a0
Compare
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.
Left more comments, happy to discuss further--think it's getting pretty close and is a major enhancement to zarf's deployment flexibility, thanks!
If the 'external' git server still exists within the k8s cluster, create a tunnel when pushing repos to it
general cleanup and oranization of functions in hooks package for zarf agent
Makes sure the svc url comes at the end of the provided URL. This captures edge cases where someone has a subdomain that looks just like a service url would. eg) https://svc.cluster.local.example.com
update GitHub workflows to not explicitly create init-package if it's not needed (if it already exists)
The get-admin-password was a bad name because it wasn't clear what 'admin' meant. We are now making the name more explicit
update the flag git-username to git-account and add a note in the usage text to describe what to do if the repository belongs under an org
verify that the git-user and git-password has been provided if using an external git server
give users the ability to set some of the flags for the GitServerInfo without needing to host an extneral repository
f8ac796
to
e9b270d
Compare
@@ -34,15 +36,28 @@ func NewGitRepositoryMutationHook() operations.Hook { | |||
func mutateGitRepository(r *v1.AdmissionRequest) (*operations.Result, error) { | |||
var patches []operations.PatchOperation | |||
|
|||
zarfState, err := getZarfStateFromFileWithinAgentPod(zarfStatePath) |
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.
Could this be shortened to just getZarfStateFromAgent
(or similar)? getZarfStateFromFileWithinAgentPod
is a little long
// Form the gitServerURL from the state | ||
gitServerURL := zarfState.GitServer.Address | ||
if zarfState.GitServer.Port != 0 { | ||
gitServerURL += fmt.Sprintf(":%d", zarfState.GitServer.Port) |
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 I said above is not true though when using a non-default port. If I hosted gittea behind an nginx server and rewrote a subpath to point to my git server then this would result in something like: https://example.com/my-scm:5000...
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 that case you could add the port as a string within the server URL and just not populate the port flag. If we ever actually need the port we could try to parse the port using net/url
.
@Racer159 Do you think this works or do you think we need to come up with a more robust solution?
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 think that works, but at that point why specify the port as a separate flag at all?
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.
That's a good point, I thought it would have been needed earlier but I'll just give the url
Closing this as it's being duplicated by #666 along with other features (handling an external container registry) |
Fixes #570