-
Notifications
You must be signed in to change notification settings - Fork 62
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
Allow for fetching packages from remote git repositories #255
Conversation
d289b8a
to
b64b627
Compare
Seeing some weirdness around re-cloning repositories with depth 1. Seems like it's a bug fixed in a release later than what we are using. I'll need to update it. |
Signed-off-by: Manabu McCloskey <[email protected]>
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.
Thanks for this left a few comments.
I think we should also update the README to go with it. Can you please make the necessary changes?
hash, push, err := addAllAndCommit(repo.Spec.Source.Path, tgtRepository) | ||
if err != nil { | ||
return fmt.Errorf("pushing to git: %w", err) | ||
return fmt.Errorf("add and commit %w", err) | ||
} | ||
|
||
if push { | ||
remoteUrl, err := util.FirstRemoteURL(tgtRepository) | ||
if err != nil { | ||
return fmt.Errorf("getting remote url %w", err) | ||
} | ||
|
||
logger.V(1).Info("pushing to remote url %s", remoteUrl) | ||
err = pushToRemote(ctx, tgtRepository, creds) | ||
if err != nil { | ||
return fmt.Errorf("pushing to git: %w", err) | ||
} | ||
|
||
repo.Status.LatestCommit.Hash = hash.String() | ||
return nil | ||
} | ||
|
||
repo.Status.LatestCommit.Hash = commit.String() | ||
repo.Status.LatestCommit.Hash = hash.String() | ||
return nil |
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.
there is some redundancy between reconcileLocalRepoContent
and reconcileRemoteRepoContent
. Can we pull them into one function for clarity?
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 does looks similar but I don't think introducing another function would save us many lines. Maybe like 3 lines. I think it's pretty clear what these functions are doing right now and I'd rather keep it that way.
Signed-off-by: Manabu McCloskey <[email protected]>
Updated Readme and addressed review comments. @nimakaviani |
README.md
Outdated
Idpbuilder supports specifying custom packages using the flag `--package-dir` flag. | ||
This flag expects a directory (local or remote) containing ArgoCD application files. | ||
In case of a remote directory, it must be a directory in a git repository, | ||
and the URL format must be a [kustommize remote URL format](https://github.com/kubernetes-sigs/kustomize/blob/master/examples/remoteBuild.md). |
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.
Typo error: kustommize
=> kustomize
Why do you force the users to use a kustomize URL instead of HTTPS or SSH git urls ? What is the added value (if there is) to use kustomize url here ?
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.
Gosh I really should configure my code spell plugin my spelling isn't good.
It actually supports git SSH format. The problem with just plain HTTPS format is that there's no way for us to know where the repository URL ends. For GitHub, it's almost always org-name/repo-name
, but that's not the case for other git providers. This is the main reason for me to use the kustomize format.
Signed-off-by: Manabu McCloskey <[email protected]>
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.
given that we have #264 to add a new flag to handle package references, I am going to approve and merge this, so we can unblock the rest of the work.
This allows for fetching packages from remote git repositories.
The URL format follows kustomize remote url format.
fixes: #184
example:
This should also reduce the number of git clone operations against local and remote repositories.