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

feat: add ability to specify a different write and base branch #304

Merged
merged 11 commits into from
Jan 7, 2022

Conversation

nkkowa
Copy link
Contributor

@nkkowa nkkowa commented Nov 25, 2021

This add new functionality by adding a write-branch to argocd-image-updater.argoproj.io/git-branch using a separator, which will be used to push back changes to the target repository. The branch from the Application's spec or specified with the existing git-branch annotation will be used as the base branch for checkout.

Further, this annotation can use Golang templating to allow for dynamic branch names based off of the changes.

This could integrate with various CI/CD (such as GitHub Actions) to automatically create a pull request.

This varies slightly from the suggestion in #198, though sill resolves the issue. Rather than adding a new annotation to specify the base branch, we're specifying a write branch. This decision was made for ease of implementation, as rather than having to add a new annotation and change an existing one, a user will only need to add a new annotation.

@CLAassistant
Copy link

CLAassistant commented Nov 25, 2021

CLA assistant check
All committers have signed the CLA.

@jannfis
Copy link
Contributor

jannfis commented Dec 16, 2021

Thanks @nick-homex and apologies for coming back so late to this PR! It indeed looks useful. I only have minor concerns: Usually, Git branch names have some limit for their length, depending on the Git implementation (I think, GitHub for example maxes out at 255 characters).

Should we take this into consideration when creating the branch name? Maybe use the template you implement as the input for a simple hash algorithm such as SHA256, and use its fixed length hex representation as the name of the branch to create? This way, branch name would stay unique for each updated images combination possible, while not running risk of exceeding the branch name's length restriction.

@nkkowa
Copy link
Contributor Author

nkkowa commented Dec 31, 2021

Thanks @nick-homex and apologies for coming back so late to this PR! It indeed looks useful. I only have minor concerns: Usually, Git branch names have some limit for their length, depending on the Git implementation (I think, GitHub for example maxes out at 255 characters).

Should we take this into consideration when creating the branch name? Maybe use the template you implement as the input for a simple hash algorithm such as SHA256, and use its fixed length hex representation as the name of the branch to create? This way, branch name would stay unique for each updated images combination possible, while not running risk of exceeding the branch name's length restriction.

Hey! Sorry for the late response, was on holidays. Thanks for the review. Perhaps add the hashed value as a top-level template variable that they could use instead? That way we leave it up to the user if they want to use a guaranteed unique branch name, or if they would prefer one that reflects the images changed. This leaves some flexibility, and as a safe measure we can just truncate the branch name if it is above the limit (this can be noted in the docs).

@nkkowa
Copy link
Contributor Author

nkkowa commented Jan 4, 2022

@jannfis I've added a SHA1 hash as a template option and truncated the length to 255 if it exceeds that, let me know if you can think of any other issues!

Copy link
Contributor

@jannfis jannfis left a comment

Choose a reason for hiding this comment

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

Thanks for the update, @nick-homex !

I have two comments that need to be addressed, please check below.

Also, I was thinking about the following: Instead of introducing a new annotation for the write-back branch, how about making this configuration part of the git-branch annotation, e.g.:

argocd-image-updater.argoproj.io/alias.git-branch: <checkout branch>:<write-branch>

AFAIK, the colon : is not a valid character for Git branch names, so could be used as a separator. Behavior would be as follows:

  1. Use checkout branch from Argo CD application, and no write back branch: Annotation not specified or empty value
  2. Use specific checkout branch without write-back-branch: `somebranch"
  3. Use specific checkout branch with write-back-branch: somebranch:otherbranch
  4. Use checkout branch from Argo CD application with write-back-branch: :otherbranch

I'm not quite sure from the usability point of view here. But we're trying to refrain from new annotations when possible, just to prevent something like annotation pollution in the Application CR.

I'm happy to merge this PR without above mentioned thing changed, however. Just some food for thought, and discussion.

pkg/argocd/git.go Outdated Show resolved Hide resolved
pkg/argocd/git.go Outdated Show resolved Hide resolved
pkg/argocd/update.go Outdated Show resolved Hide resolved
@nkkowa
Copy link
Contributor Author

nkkowa commented Jan 6, 2022

@jannfis Done the requested changes, good idea on using one annotation and the separator.

@nkkowa nkkowa requested a review from jannfis January 6, 2022 17:23
Copy link
Contributor

@jannfis jannfis left a comment

Choose a reason for hiding this comment

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

LGTM!

Thanks for this awesome contribution, @nick-homex!

@jannfis jannfis changed the title add ability to specify a different write and base branch feat: add ability to specify a different write and base branch Jan 7, 2022
@jannfis jannfis merged commit a374b73 into argoproj-labs:master Jan 7, 2022
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.

None yet

3 participants