Add support for incremental stamping - #175

Merged
kzadorozhny merged 13 commits into
mainfrom
randrei/incrementalStamping
Jul 8, 2024
Merged

Add support for incremental stamping#175
kzadorozhny merged 13 commits into
mainfrom
randrei/incrementalStamping

Conversation

@randrei-adobe

@randrei-adoberandrei-adobe commented Apr 29, 2024

Copy link
Copy Markdown
Member

Description

Modify the create_gitops_pr utility to optionally implement the following algorithm:

  • Assume the service.yaml file is an output of the service.gitops target
  • Run service.gitops > service.yaml
  • Calculate service.yaml digest > service.yaml.digest
  • Check if the contents of the service.yaml.digest are the same as those committed into git
    • Stop if there are no changes to the service.yaml.digest file
  • Stamp (replace placeholders) service.yaml
  • Commit and push the changes to service.yaml.digest and service.yaml

Related 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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Comment threadskylib/kustomize/kustomize.bzl Outdated
variables = "--variable=NAMESPACE={namespace}".format(
namespace = namespace,
)
variables += " --variable=GIT_REVISION=\"$(git rev-parse HEAD)\""

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should not do stamping of stamps in .show and .apply commands.

  1. The git command will not work because the current directory is not the source directory.
  2. We will be breaking the idempotency properties of .show and apply commands, which is no backward compatible behavior change.

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")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment threadgitops/git/git.go Outdated
}

// split by newline and ignore empty strings
func SplitFunc(c rune) bool {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This function doesn't seem to belong to the public interface of git module. Please make it private or inline.

Comment threadgitops/git/git.go Outdated
if err != nil {
log.Fatalf("ERROR: %s", err)
}
return strings.FieldsFunc(files, SplitFunc)

@kzadorozhnykzadorozhnyApr 29, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

varfiles []stringsc:=bufio.NewScanner(strings.NewReader(s))
forsc.Scan() {
files=append(files, sc.Text())
}
returnlines

Comment threadgitops/git/git.go Outdated
}

// RemoveDiff removes the changes made to a specific file in the repository
func (r *Repo) RemoveDiff(fileName string) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
func (r*Repo) RemoveDiff(fileNamestring) {
func (r*Repo) RestoreFile(fileNamestring) {

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

Comment threadgitops/prer/create_gitops_prs.go Outdated
return qr
}

func getContext(workdir *git.Repo, branchName string) map[string]interface{} {

@kzadorozhnykzadorozhnyApr 29, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
funcgetContext(workdir*git.Repo, branchNamestring) map[string]interface{} {
funcgetGitStatusDict(workdir*git.Repo, branchNamestring) map[string]interface{} {

getContext is too generic.

Comment threadgitops/prer/create_gitops_prs.go Outdated

ctx := getContext(workdir, branchName)

stampedTemplate := fasttemplate.ExecuteString(string(template), "{{", "}}", ctx)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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)
iferr!=nil {
log.Fatalf("Unable to create output file %s: %v", output, err)
}
deferoutf.Close()
_, err=fasttemplate.Execute(string(template), "{{", "}}", ctx)

@randrei-adobe
randrei-adobe marked this pull request as ready for review May 2, 2024 09:02
@randrei-adobe
randrei-adobe requested a review from a team as a code ownerMay 2, 2024 09:02
@kzadorozhny
kzadorozhny merged commit 5bcb981 into mainJul 8, 2024
Sign up for freeto 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.

2 participants

@randrei-adobe@kzadorozhny
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Add support for incremental stamping - #175

Merged
kzadorozhny merged 13 commits into
mainfrom
randrei/incrementalStamping
Jul 8, 2024
Merged

Add support for incremental stamping#175
kzadorozhny merged 13 commits into
mainfrom
randrei/incrementalStamping

Conversation

@randrei-adobe

@randrei-adoberandrei-adobe commented Apr 29, 2024

Copy link
Copy Markdown
Member

Description

Modify the create_gitops_pr utility to optionally implement the following algorithm:

  • Assume the service.yaml file is an output of the service.gitops target
  • Run service.gitops > service.yaml
  • Calculate service.yaml digest > service.yaml.digest
  • Check if the contents of the service.yaml.digest are the same as those committed into git
    • Stop if there are no changes to the service.yaml.digest file
  • Stamp (replace placeholders) service.yaml
  • Commit and push the changes to service.yaml.digest and service.yaml

Related 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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Comment threadskylib/kustomize/kustomize.bzl Outdated
variables = "--variable=NAMESPACE={namespace}".format(
namespace = namespace,
)
variables += " --variable=GIT_REVISION=\"$(git rev-parse HEAD)\""

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should not do stamping of stamps in .show and .apply commands.

  1. The git command will not work because the current directory is not the source directory.
  2. We will be breaking the idempotency properties of .show and apply commands, which is no backward compatible behavior change.

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")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment threadgitops/git/git.go Outdated
}

// split by newline and ignore empty strings
func SplitFunc(c rune) bool {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This function doesn't seem to belong to the public interface of git module. Please make it private or inline.

Comment threadgitops/git/git.go Outdated
if err != nil {
log.Fatalf("ERROR: %s", err)
}
return strings.FieldsFunc(files, SplitFunc)

@kzadorozhnykzadorozhnyApr 29, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

varfiles []stringsc:=bufio.NewScanner(strings.NewReader(s))
forsc.Scan() {
files=append(files, sc.Text())
}
returnlines

Comment threadgitops/git/git.go Outdated
}

// RemoveDiff removes the changes made to a specific file in the repository
func (r *Repo) RemoveDiff(fileName string) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
func (r*Repo) RemoveDiff(fileNamestring) {
func (r*Repo) RestoreFile(fileNamestring) {

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

Comment threadgitops/prer/create_gitops_prs.go Outdated
return qr
}

func getContext(workdir *git.Repo, branchName string) map[string]interface{} {

@kzadorozhnykzadorozhnyApr 29, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
funcgetContext(workdir*git.Repo, branchNamestring) map[string]interface{} {
funcgetGitStatusDict(workdir*git.Repo, branchNamestring) map[string]interface{} {

getContext is too generic.

Comment threadgitops/prer/create_gitops_prs.go Outdated

ctx := getContext(workdir, branchName)

stampedTemplate := fasttemplate.ExecuteString(string(template), "{{", "}}", ctx)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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)
iferr!=nil {
log.Fatalf("Unable to create output file %s: %v", output, err)
}
deferoutf.Close()
_, err=fasttemplate.Execute(string(template), "{{", "}}", ctx)

@randrei-adobe
randrei-adobe marked this pull request as ready for review May 2, 2024 09:02
@randrei-adobe
randrei-adobe requested a review from a team as a code ownerMay 2, 2024 09:02
@kzadorozhny
kzadorozhny merged commit 5bcb981 into mainJul 8, 2024
Sign up for freeto 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.

2 participants

@randrei-adobe@kzadorozhny
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Add support for incremental stamping - #175

Merged
kzadorozhny merged 13 commits into
mainfrom
randrei/incrementalStamping
Jul 8, 2024
Merged

Add support for incremental stamping#175
kzadorozhny merged 13 commits into
mainfrom
randrei/incrementalStamping

Conversation

@randrei-adobe

@randrei-adoberandrei-adobe commented Apr 29, 2024

Copy link
Copy Markdown
Member

Description

Modify the create_gitops_pr utility to optionally implement the following algorithm:

  • Assume the service.yaml file is an output of the service.gitops target
  • Run service.gitops > service.yaml
  • Calculate service.yaml digest > service.yaml.digest
  • Check if the contents of the service.yaml.digest are the same as those committed into git
    • Stop if there are no changes to the service.yaml.digest file
  • Stamp (replace placeholders) service.yaml
  • Commit and push the changes to service.yaml.digest and service.yaml

Related 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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Comment threadskylib/kustomize/kustomize.bzl Outdated
variables = "--variable=NAMESPACE={namespace}".format(
namespace = namespace,
)
variables += " --variable=GIT_REVISION=\"$(git rev-parse HEAD)\""

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should not do stamping of stamps in .show and .apply commands.

  1. The git command will not work because the current directory is not the source directory.
  2. We will be breaking the idempotency properties of .show and apply commands, which is no backward compatible behavior change.

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")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment threadgitops/git/git.go Outdated
}

// split by newline and ignore empty strings
func SplitFunc(c rune) bool {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This function doesn't seem to belong to the public interface of git module. Please make it private or inline.

Comment threadgitops/git/git.go Outdated
if err != nil {
log.Fatalf("ERROR: %s", err)
}
return strings.FieldsFunc(files, SplitFunc)

@kzadorozhnykzadorozhnyApr 29, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

varfiles []stringsc:=bufio.NewScanner(strings.NewReader(s))
forsc.Scan() {
files=append(files, sc.Text())
}
returnlines

Comment threadgitops/git/git.go Outdated
}

// RemoveDiff removes the changes made to a specific file in the repository
func (r *Repo) RemoveDiff(fileName string) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
func (r*Repo) RemoveDiff(fileNamestring) {
func (r*Repo) RestoreFile(fileNamestring) {

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

Comment threadgitops/prer/create_gitops_prs.go Outdated
return qr
}

func getContext(workdir *git.Repo, branchName string) map[string]interface{} {

@kzadorozhnykzadorozhnyApr 29, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
funcgetContext(workdir*git.Repo, branchNamestring) map[string]interface{} {
funcgetGitStatusDict(workdir*git.Repo, branchNamestring) map[string]interface{} {

getContext is too generic.

Comment threadgitops/prer/create_gitops_prs.go Outdated

ctx := getContext(workdir, branchName)

stampedTemplate := fasttemplate.ExecuteString(string(template), "{{", "}}", ctx)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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)
iferr!=nil {
log.Fatalf("Unable to create output file %s: %v", output, err)
}
deferoutf.Close()
_, err=fasttemplate.Execute(string(template), "{{", "}}", ctx)

@randrei-adobe
randrei-adobe marked this pull request as ready for review May 2, 2024 09:02
@randrei-adobe
randrei-adobe requested a review from a team as a code ownerMay 2, 2024 09:02
@kzadorozhny
kzadorozhny merged commit 5bcb981 into mainJul 8, 2024
Sign up for freeto 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.

2 participants

@randrei-adobe@kzadorozhny
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Add support for incremental stamping - #175

Merged
kzadorozhny merged 13 commits into
mainfrom
randrei/incrementalStamping
Jul 8, 2024
Merged

Add support for incremental stamping#175
kzadorozhny merged 13 commits into
mainfrom
randrei/incrementalStamping

Conversation

@randrei-adobe

@randrei-adoberandrei-adobe commented Apr 29, 2024

Copy link
Copy Markdown
Member

Description

Modify the create_gitops_pr utility to optionally implement the following algorithm:

  • Assume the service.yaml file is an output of the service.gitops target
  • Run service.gitops > service.yaml
  • Calculate service.yaml digest > service.yaml.digest
  • Check if the contents of the service.yaml.digest are the same as those committed into git
    • Stop if there are no changes to the service.yaml.digest file
  • Stamp (replace placeholders) service.yaml
  • Commit and push the changes to service.yaml.digest and service.yaml

Related 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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Comment threadskylib/kustomize/kustomize.bzl Outdated
variables = "--variable=NAMESPACE={namespace}".format(
namespace = namespace,
)
variables += " --variable=GIT_REVISION=\"$(git rev-parse HEAD)\""

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should not do stamping of stamps in .show and .apply commands.

  1. The git command will not work because the current directory is not the source directory.
  2. We will be breaking the idempotency properties of .show and apply commands, which is no backward compatible behavior change.

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")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment threadgitops/git/git.go Outdated
}

// split by newline and ignore empty strings
func SplitFunc(c rune) bool {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This function doesn't seem to belong to the public interface of git module. Please make it private or inline.

Comment threadgitops/git/git.go Outdated
if err != nil {
log.Fatalf("ERROR: %s", err)
}
return strings.FieldsFunc(files, SplitFunc)

@kzadorozhnykzadorozhnyApr 29, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

varfiles []stringsc:=bufio.NewScanner(strings.NewReader(s))
forsc.Scan() {
files=append(files, sc.Text())
}
returnlines

Comment threadgitops/git/git.go Outdated
}

// RemoveDiff removes the changes made to a specific file in the repository
func (r *Repo) RemoveDiff(fileName string) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
func (r*Repo) RemoveDiff(fileNamestring) {
func (r*Repo) RestoreFile(fileNamestring) {

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

Comment threadgitops/prer/create_gitops_prs.go Outdated
return qr
}

func getContext(workdir *git.Repo, branchName string) map[string]interface{} {

@kzadorozhnykzadorozhnyApr 29, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
funcgetContext(workdir*git.Repo, branchNamestring) map[string]interface{} {
funcgetGitStatusDict(workdir*git.Repo, branchNamestring) map[string]interface{} {

getContext is too generic.

Comment threadgitops/prer/create_gitops_prs.go Outdated

ctx := getContext(workdir, branchName)

stampedTemplate := fasttemplate.ExecuteString(string(template), "{{", "}}", ctx)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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)
iferr!=nil {
log.Fatalf("Unable to create output file %s: %v", output, err)
}
deferoutf.Close()
_, err=fasttemplate.Execute(string(template), "{{", "}}", ctx)

@randrei-adobe
randrei-adobe marked this pull request as ready for review May 2, 2024 09:02
@randrei-adobe
randrei-adobe requested a review from a team as a code ownerMay 2, 2024 09:02
@kzadorozhny
kzadorozhny merged commit 5bcb981 into mainJul 8, 2024
Sign up for freeto 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.

2 participants

@randrei-adobe@kzadorozhny
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Add support for incremental stamping - #175

Merged
kzadorozhny merged 13 commits into
mainfrom
randrei/incrementalStamping
Jul 8, 2024
Merged

Add support for incremental stamping#175
kzadorozhny merged 13 commits into
mainfrom
randrei/incrementalStamping

Conversation

@randrei-adobe

@randrei-adoberandrei-adobe commented Apr 29, 2024

Copy link
Copy Markdown
Member

Description

Modify the create_gitops_pr utility to optionally implement the following algorithm:

  • Assume the service.yaml file is an output of the service.gitops target
  • Run service.gitops > service.yaml
  • Calculate service.yaml digest > service.yaml.digest
  • Check if the contents of the service.yaml.digest are the same as those committed into git
    • Stop if there are no changes to the service.yaml.digest file
  • Stamp (replace placeholders) service.yaml
  • Commit and push the changes to service.yaml.digest and service.yaml

Related 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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Comment threadskylib/kustomize/kustomize.bzl Outdated
variables = "--variable=NAMESPACE={namespace}".format(
namespace = namespace,
)
variables += " --variable=GIT_REVISION=\"$(git rev-parse HEAD)\""

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should not do stamping of stamps in .show and .apply commands.

  1. The git command will not work because the current directory is not the source directory.
  2. We will be breaking the idempotency properties of .show and apply commands, which is no backward compatible behavior change.

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")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment threadgitops/git/git.go Outdated
}

// split by newline and ignore empty strings
func SplitFunc(c rune) bool {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This function doesn't seem to belong to the public interface of git module. Please make it private or inline.

Comment threadgitops/git/git.go Outdated
if err != nil {
log.Fatalf("ERROR: %s", err)
}
return strings.FieldsFunc(files, SplitFunc)

@kzadorozhnykzadorozhnyApr 29, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

varfiles []stringsc:=bufio.NewScanner(strings.NewReader(s))
forsc.Scan() {
files=append(files, sc.Text())
}
returnlines

Comment threadgitops/git/git.go Outdated
}

// RemoveDiff removes the changes made to a specific file in the repository
func (r *Repo) RemoveDiff(fileName string) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
func (r*Repo) RemoveDiff(fileNamestring) {
func (r*Repo) RestoreFile(fileNamestring) {

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

Comment threadgitops/prer/create_gitops_prs.go Outdated
return qr
}

func getContext(workdir *git.Repo, branchName string) map[string]interface{} {

@kzadorozhnykzadorozhnyApr 29, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
funcgetContext(workdir*git.Repo, branchNamestring) map[string]interface{} {
funcgetGitStatusDict(workdir*git.Repo, branchNamestring) map[string]interface{} {

getContext is too generic.

Comment threadgitops/prer/create_gitops_prs.go Outdated

ctx := getContext(workdir, branchName)

stampedTemplate := fasttemplate.ExecuteString(string(template), "{{", "}}", ctx)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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)
iferr!=nil {
log.Fatalf("Unable to create output file %s: %v", output, err)
}
deferoutf.Close()
_, err=fasttemplate.Execute(string(template), "{{", "}}", ctx)

@randrei-adobe
randrei-adobe marked this pull request as ready for review May 2, 2024 09:02
@randrei-adobe
randrei-adobe requested a review from a team as a code ownerMay 2, 2024 09:02
@kzadorozhny
kzadorozhny merged commit 5bcb981 into mainJul 8, 2024
Sign up for freeto 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.

2 participants

@randrei-adobe@kzadorozhny
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Add support for incremental stamping - #175

Merged
kzadorozhny merged 13 commits into
mainfrom
randrei/incrementalStamping
Jul 8, 2024
Merged

Add support for incremental stamping#175
kzadorozhny merged 13 commits into
mainfrom
randrei/incrementalStamping

Conversation

@randrei-adobe

@randrei-adoberandrei-adobe commented Apr 29, 2024

Copy link
Copy Markdown
Member

Description

Modify the create_gitops_pr utility to optionally implement the following algorithm:

  • Assume the service.yaml file is an output of the service.gitops target
  • Run service.gitops > service.yaml
  • Calculate service.yaml digest > service.yaml.digest
  • Check if the contents of the service.yaml.digest are the same as those committed into git
    • Stop if there are no changes to the service.yaml.digest file
  • Stamp (replace placeholders) service.yaml
  • Commit and push the changes to service.yaml.digest and service.yaml

Related 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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Comment threadskylib/kustomize/kustomize.bzl Outdated
variables = "--variable=NAMESPACE={namespace}".format(
namespace = namespace,
)
variables += " --variable=GIT_REVISION=\"$(git rev-parse HEAD)\""

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should not do stamping of stamps in .show and .apply commands.

  1. The git command will not work because the current directory is not the source directory.
  2. We will be breaking the idempotency properties of .show and apply commands, which is no backward compatible behavior change.

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")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment threadgitops/git/git.go Outdated
}

// split by newline and ignore empty strings
func SplitFunc(c rune) bool {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This function doesn't seem to belong to the public interface of git module. Please make it private or inline.

Comment threadgitops/git/git.go Outdated
if err != nil {
log.Fatalf("ERROR: %s", err)
}
return strings.FieldsFunc(files, SplitFunc)

@kzadorozhnykzadorozhnyApr 29, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

varfiles []stringsc:=bufio.NewScanner(strings.NewReader(s))
forsc.Scan() {
files=append(files, sc.Text())
}
returnlines

Comment threadgitops/git/git.go Outdated
}

// RemoveDiff removes the changes made to a specific file in the repository
func (r *Repo) RemoveDiff(fileName string) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
func (r*Repo) RemoveDiff(fileNamestring) {
func (r*Repo) RestoreFile(fileNamestring) {

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

Comment threadgitops/prer/create_gitops_prs.go Outdated
return qr
}

func getContext(workdir *git.Repo, branchName string) map[string]interface{} {

@kzadorozhnykzadorozhnyApr 29, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
funcgetContext(workdir*git.Repo, branchNamestring) map[string]interface{} {
funcgetGitStatusDict(workdir*git.Repo, branchNamestring) map[string]interface{} {

getContext is too generic.

Comment threadgitops/prer/create_gitops_prs.go Outdated

ctx := getContext(workdir, branchName)

stampedTemplate := fasttemplate.ExecuteString(string(template), "{{", "}}", ctx)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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)
iferr!=nil {
log.Fatalf("Unable to create output file %s: %v", output, err)
}
deferoutf.Close()
_, err=fasttemplate.Execute(string(template), "{{", "}}", ctx)

@randrei-adobe
randrei-adobe marked this pull request as ready for review May 2, 2024 09:02
@randrei-adobe
randrei-adobe requested a review from a team as a code ownerMay 2, 2024 09:02
@kzadorozhny
kzadorozhny merged commit 5bcb981 into mainJul 8, 2024
Sign up for freeto 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.

2 participants

@randrei-adobe@kzadorozhny
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Add support for incremental stamping - #175

Merged
kzadorozhny merged 13 commits into
mainfrom
randrei/incrementalStamping
Jul 8, 2024
Merged

Add support for incremental stamping#175
kzadorozhny merged 13 commits into
mainfrom
randrei/incrementalStamping

Conversation

@randrei-adobe

@randrei-adoberandrei-adobe commented Apr 29, 2024

Copy link
Copy Markdown
Member

Description

Modify the create_gitops_pr utility to optionally implement the following algorithm:

  • Assume the service.yaml file is an output of the service.gitops target
  • Run service.gitops > service.yaml
  • Calculate service.yaml digest > service.yaml.digest
  • Check if the contents of the service.yaml.digest are the same as those committed into git
    • Stop if there are no changes to the service.yaml.digest file
  • Stamp (replace placeholders) service.yaml
  • Commit and push the changes to service.yaml.digest and service.yaml

Related 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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Comment threadskylib/kustomize/kustomize.bzl Outdated
variables = "--variable=NAMESPACE={namespace}".format(
namespace = namespace,
)
variables += " --variable=GIT_REVISION=\"$(git rev-parse HEAD)\""

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should not do stamping of stamps in .show and .apply commands.

  1. The git command will not work because the current directory is not the source directory.
  2. We will be breaking the idempotency properties of .show and apply commands, which is no backward compatible behavior change.

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")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment threadgitops/git/git.go Outdated
}

// split by newline and ignore empty strings
func SplitFunc(c rune) bool {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This function doesn't seem to belong to the public interface of git module. Please make it private or inline.

Comment threadgitops/git/git.go Outdated
if err != nil {
log.Fatalf("ERROR: %s", err)
}
return strings.FieldsFunc(files, SplitFunc)

@kzadorozhnykzadorozhnyApr 29, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

varfiles []stringsc:=bufio.NewScanner(strings.NewReader(s))
forsc.Scan() {
files=append(files, sc.Text())
}
returnlines

Comment threadgitops/git/git.go Outdated
}

// RemoveDiff removes the changes made to a specific file in the repository
func (r *Repo) RemoveDiff(fileName string) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
func (r*Repo) RemoveDiff(fileNamestring) {
func (r*Repo) RestoreFile(fileNamestring) {

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

Comment threadgitops/prer/create_gitops_prs.go Outdated
return qr
}

func getContext(workdir *git.Repo, branchName string) map[string]interface{} {

@kzadorozhnykzadorozhnyApr 29, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
funcgetContext(workdir*git.Repo, branchNamestring) map[string]interface{} {
funcgetGitStatusDict(workdir*git.Repo, branchNamestring) map[string]interface{} {

getContext is too generic.

Comment threadgitops/prer/create_gitops_prs.go Outdated

ctx := getContext(workdir, branchName)

stampedTemplate := fasttemplate.ExecuteString(string(template), "{{", "}}", ctx)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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)
iferr!=nil {
log.Fatalf("Unable to create output file %s: %v", output, err)
}
deferoutf.Close()
_, err=fasttemplate.Execute(string(template), "{{", "}}", ctx)

@randrei-adobe
randrei-adobe marked this pull request as ready for review May 2, 2024 09:02
@randrei-adobe
randrei-adobe requested a review from a team as a code ownerMay 2, 2024 09:02
@kzadorozhny
kzadorozhny merged commit 5bcb981 into mainJul 8, 2024
Sign up for freeto 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.

2 participants

@randrei-adobe@kzadorozhny
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Add support for incremental stamping - #175

Merged
kzadorozhny merged 13 commits into
mainfrom
randrei/incrementalStamping
Jul 8, 2024
Merged

Add support for incremental stamping#175
kzadorozhny merged 13 commits into
mainfrom
randrei/incrementalStamping

Conversation

@randrei-adobe

@randrei-adoberandrei-adobe commented Apr 29, 2024

Copy link
Copy Markdown
Member

Description

Modify the create_gitops_pr utility to optionally implement the following algorithm:

  • Assume the service.yaml file is an output of the service.gitops target
  • Run service.gitops > service.yaml
  • Calculate service.yaml digest > service.yaml.digest
  • Check if the contents of the service.yaml.digest are the same as those committed into git
    • Stop if there are no changes to the service.yaml.digest file
  • Stamp (replace placeholders) service.yaml
  • Commit and push the changes to service.yaml.digest and service.yaml

Related 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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Comment threadskylib/kustomize/kustomize.bzl Outdated
variables = "--variable=NAMESPACE={namespace}".format(
namespace = namespace,
)
variables += " --variable=GIT_REVISION=\"$(git rev-parse HEAD)\""

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should not do stamping of stamps in .show and .apply commands.

  1. The git command will not work because the current directory is not the source directory.
  2. We will be breaking the idempotency properties of .show and apply commands, which is no backward compatible behavior change.

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")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment threadgitops/git/git.go Outdated
}

// split by newline and ignore empty strings
func SplitFunc(c rune) bool {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This function doesn't seem to belong to the public interface of git module. Please make it private or inline.

Comment threadgitops/git/git.go Outdated
if err != nil {
log.Fatalf("ERROR: %s", err)
}
return strings.FieldsFunc(files, SplitFunc)

@kzadorozhnykzadorozhnyApr 29, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

varfiles []stringsc:=bufio.NewScanner(strings.NewReader(s))
forsc.Scan() {
files=append(files, sc.Text())
}
returnlines

Comment threadgitops/git/git.go Outdated
}

// RemoveDiff removes the changes made to a specific file in the repository
func (r *Repo) RemoveDiff(fileName string) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
func (r*Repo) RemoveDiff(fileNamestring) {
func (r*Repo) RestoreFile(fileNamestring) {

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

Comment threadgitops/prer/create_gitops_prs.go Outdated
return qr
}

func getContext(workdir *git.Repo, branchName string) map[string]interface{} {

@kzadorozhnykzadorozhnyApr 29, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
funcgetContext(workdir*git.Repo, branchNamestring) map[string]interface{} {
funcgetGitStatusDict(workdir*git.Repo, branchNamestring) map[string]interface{} {

getContext is too generic.

Comment threadgitops/prer/create_gitops_prs.go Outdated

ctx := getContext(workdir, branchName)

stampedTemplate := fasttemplate.ExecuteString(string(template), "{{", "}}", ctx)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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)
iferr!=nil {
log.Fatalf("Unable to create output file %s: %v", output, err)
}
deferoutf.Close()
_, err=fasttemplate.Execute(string(template), "{{", "}}", ctx)

@randrei-adobe
randrei-adobe marked this pull request as ready for review May 2, 2024 09:02
@randrei-adobe
randrei-adobe requested a review from a team as a code ownerMay 2, 2024 09:02
@kzadorozhny
kzadorozhny merged commit 5bcb981 into mainJul 8, 2024
Sign up for freeto 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.

2 participants

@randrei-adobe@kzadorozhny