Add support for incremental stamping #175
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kzadorozhny
reviewed
Apr 29, 2024
skylib/kustomize/kustomize.bzl
Outdated
| variables = "--variable=NAMESPACE={namespace}".format( | ||
| namespace = namespace, | ||
| ) | ||
| variables += " --variable=GIT_REVISION=\"$(git rev-parse HEAD)\"" |
Member
There was a problem hiding this comment.
We should not do stamping of stamps in .show and .apply commands.
- The git command will not work because the current directory is not the source directory.
- We will be breaking the idempotency properties of
.showandapplycommands, which is no backward compatible behavior change.
kzadorozhny
reviewed
Apr 29, 2024
Comment on lines
75
to
76
| dryRun = flag.Bool("dry_run", false, "Do not create PRs, just print what would be done") | ||
| stamp = flag.Bool("stamp", false, "Stamp results of gitops targets with volatile information") |
Member
There was a problem hiding this comment.
Suggested change
| dryRun = flag.Bool("dry_run", false, "Do not create PRs, just print what would be done") | |
| stamp = flag.Bool("stamp", false, "Stamp results of gitops targets with volatile information") | |
| stamp = flag.Bool("stamp", false, "Stamp results of gitops targets with volatile information") | |
| dryRun = flag.Bool("dry_run", false, "Do not create PRs, just print what would be done") |
Historically we kept dryRun as a last argument definition.
kzadorozhny
reviewed
Apr 29, 2024
gitops/git/git.go
Outdated
| } | ||
|
|
||
| // split by newline and ignore empty strings | ||
| func SplitFunc(c rune) bool { |
Member
There was a problem hiding this comment.
This function doesn't seem to belong to the public interface of git module. Please make it private or inline.
kzadorozhny
reviewed
Apr 29, 2024
gitops/git/git.go
Outdated
| if err != nil { | ||
| log.Fatalf("ERROR: %s", err) | ||
| } | ||
| return strings.FieldsFunc(files, SplitFunc) |
Member
There was a problem hiding this comment.
I'd prefer the use of the Scanner. While it will be more verbose it will be more obvious what empty lines re not returned.
var files []string
sc := bufio.NewScanner(strings.NewReader(s))
for sc.Scan() {
files = append(files, sc.Text())
}
return lines
kzadorozhny
reviewed
Apr 29, 2024
gitops/git/git.go
Outdated
| } | ||
|
|
||
| // RemoveDiff removes the changes made to a specific file in the repository | ||
| func (r *Repo) RemoveDiff(fileName string) { |
Member
There was a problem hiding this comment.
Suggested change
| func (r *Repo) RemoveDiff(fileName string) { | |
| func (r *Repo) RestoreFile(fileName string) { |
Align with the terminology used by Git. For example:
Changes not staged for commit:
(use "git add <file>..." to update what will be committed)
(use "git restore <file>..." to discard changes in working directory)
modified: src/index.ts
kzadorozhny
reviewed
Apr 29, 2024
gitops/prer/create_gitops_prs.go
Outdated
| return qr | ||
| } | ||
|
|
||
| func getContext(workdir *git.Repo, branchName string) map[string]interface{} { |
Member
There was a problem hiding this comment.
Suggested change
| func getContext(workdir *git.Repo, branchName string) map[string]interface{} { | |
| func getGitStatusDict(workdir *git.Repo, branchName string) map[string]interface{} { |
getContext is too generic.
kzadorozhny
reviewed
Apr 29, 2024
gitops/prer/create_gitops_prs.go
Outdated
|
|
||
| ctx := getContext(workdir, branchName) | ||
|
|
||
| stampedTemplate := fasttemplate.ExecuteString(string(template), "{{", "}}", ctx) |
Member
There was a problem hiding this comment.
fasttemplate.Execute function writes directly into file.
Suggested change
| stampedTemplate := fasttemplate.ExecuteString(string(template), "{{", "}}", ctx) | |
| outf, err = os.OpenFile(fullPath, os.O_RDWR|os.O_CREATE|os.O_TRUNC, perm) | |
| if err != nil { | |
| log.Fatalf("Unable to create output file %s: %v", output, err) | |
| } | |
| defer outf.Close() | |
| _, err = fasttemplate.Execute(string(template), "{{", "}}", ctx) |
kzadorozhny
approved these changes
Jul 8, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Modify the
create_gitops_prutility to optionally implement the following algorithm:service.yamlfile is an output of theservice.gitopstargetservice.gitops>service.yamlservice.yamldigest >service.yaml.digestservice.yaml.digestare the same as those committed into gitservice.yaml.digestfileservice.yamlservice.yaml.digestandservice.yamlRelated Issue
Motivation and Context
The existing GitOps PR process creates a new GitOps PR when a ConfigMap object that exposes a volatile file changes even if the deployment artifact doesn't change.
How Has This Been Tested?
Types of changes
Checklist: