Update artifact tarball when `spec.ignore` changes #712

angelokurtis · 2022-05-06T17:17:50Z

This commit adds a new field status.artifact.excludedPatternsChecksum which is used to detect changes on spec.ignore, that controls the exclusions for the object's status.artifact.

Fix: #704

darkowlzz · 2022-05-09T10:18:58Z

Hi, thanks for proposing this solution. Sharing a few thoughts about it:

Maybe it'd be better to not mix the terms. The spec has it as ignore, it'd be better to call the new field with the same name.
Ignore is an attribute of GitRepo and Bucket only. Adding the checksum field as part of artifact exposes it to all the other source types. Maybe it'd be better to introduce it in the status of GitRepo and Bucket only. There's already status.includedArtifacts in GitRepo. Maybe we could have ignored informtion there as well.

hiddeco · 2022-05-09T10:31:51Z

@darkowlzz you are pretty much reflecting my thoughts in #710 :-)

angelokurtis · 2022-05-09T19:29:53Z

Hey everyone! Thanks for the feedback!
I updated the PR with your suggestions. Please let me know if you see any other improvements.

darkowlzz

Thanks for updating the implementation.
Left some inline comments.
The test failure seems to be unrelated.

api/v1beta2/bucket_types.go

controllers/gitrepository_controller.go

stefanprodan · 2022-05-10T13:15:56Z

Please rebase with upstream main, we've fixed the e2e tests.

darkowlzz · 2022-05-11T08:35:01Z

controllers/bucket_controller_test.go

+			assertConditions: []metav1.Condition{
+				*conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855'"),
+			},
+		},


Nice tests.
In the above, we test for cases where ignore is added and removed. Let's also test for the case where the existing value of ignore is updated, would result in a different checksum, not empty.

Same for the GitRepo tests.

darkowlzz · 2022-05-11T08:39:35Z

controllers/gitrepository_controller.go

 		r.eventLogf(ctx, obj, events.EventTypeTrace, sourcev1.ArtifactUpToDateReason, "artifact up-to-date with remote revision: '%s'", artifact.Revision)
 		return sreconcile.ResultSuccess, nil
 	}

+	// Ensure .spec.ignore checksum is up-to-date


Maybe

Suggested change

// Ensure .spec.ignore checksum is up-to-date

// Ensure .status.ignoreChecksum is up-to-date with .spec.ignore.

Same for the Bucket code.

darkowlzz · 2022-05-11T08:44:54Z

Could you also update the spec docs in https://github.com/fluxcd/source-controller/tree/main/docs/spec/v1beta2 for both GitRepo and Bucket in the Status section towards the bottom of the docs with some documentation for the checksum field?

hiddeco · 2022-05-11T11:06:38Z

api/v1beta2/gitrepository_types.go

@@ -211,6 +211,12 @@ type GitRepositoryStatus struct {
 	// +optional
 	IncludedArtifacts []*Artifact `json:"includedArtifacts,omitempty"`

+	// IgnoreChecksum is a checksum of .spec.ignore in .status.observedGeneration
+	// version of the object.
+	// It is formatted as `<algo>:<checksum>`. For example: `sha256:<checksum>`.


Same comment.

hiddeco · 2022-05-11T11:06:42Z

api/v1beta2/bucket_types.go

@@ -124,6 +124,12 @@ type BucketStatus struct {
 	// +optional
 	Artifact *Artifact `json:"artifact,omitempty"`

+	// IgnoreChecksum is a checksum of .spec.ignore in .status.observedGeneration
+	// version of the object.
+	// It is formatted as `<algo>:<checksum>`. For example: `sha256:<checksum>`.


Suggested change

// It is formatted as `<algo>:<checksum>`. For example: `sha256:<checksum>`.

// It has the format of `<algo>:<checksum>`, for example: `sha256:<checksum>`.

hiddeco · 2022-05-11T11:07:04Z

controllers/bucket_controller.go

@@ -42,6 +41,8 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/predicate"
 	"sigs.k8s.io/controller-runtime/pkg/ratelimiter"

+	"github.com/fluxcd/source-controller/pkg/azure"


This should be moved to block on L53.

This commit adds a new field `status.ignoreChecksum` which is used to detect changes on `spec.ignore`, that controls the exclusions for the object's `status.artifact` Signed-off-by: Tiago Angelo <kurtis.angelo@gmail.com>

Signed-off-by: Tiago Angelo <kurtis.angelo@gmail.com>

hiddeco

This is code-wise looking picture perfect to me, thank you very much for being patient with our feedback 💯

One last request: could you add the fields to the spec documentation for both object kinds? A tiny description in the Status section about what it is, and what it triggers is sufficient. 🙇

Signed-off-by: Tiago Angelo <kurtis.angelo@gmail.com>

angelokurtis · 2022-05-13T19:28:15Z

Hi guys, thanks for the support!
Please, let me know if you see any other improvements.

hiddeco · 2022-05-18T06:31:07Z

docs/spec/v1beta2/buckets.md

+The source-controller calculates the SHA256 checksum of the last Bucket's
+[`.spec.ignore` field](#ignore) and record it in the `.status.ignoreChecksum` field. This field
+is used to detect `.spec.ignore` updates to allow artifact changes.


Suggested change

The source-controller calculates the SHA256 checksum of the last Bucket's

[`.spec.ignore` field](#ignore) and record it in the `.status.ignoreChecksum` field. This field

is used to detect `.spec.ignore` updates to allow artifact changes.

The source-controller calculates the SHA256 checksum Bucket's [`.spec.ignore` field](#ignore),

and records it in `.status.ignoreChecksum`. This field is used to detect `.spec.ignore` changes to

trigger an Artifact update.

hiddeco · 2022-05-18T06:31:13Z

docs/spec/v1beta2/gitrepositories.md

@@ -838,6 +838,12 @@ Note that a GitRepository can be [reconciling](#reconciling-gitrepository)
 while failing at the same time, for example due to a newly introduced
 configuration issue in the GitRepository spec.

+### Ignore Checksum
+
+The source-controller calculates the SHA256 checksum of the last GitRepository's


Same suggestion applies here.

darkowlzz · 2022-05-18T07:17:51Z

Hi, due to some recent changes in the main branch, we had to restructure a lot of the code in GitRepo reconciler. A rough work on fixing that is in https://github.com/fluxcd/source-controller/compare/gitrepo-rec-fixes . It changes how we figure out if the source needs to be reconciled completely or not, similar to detecting changes to spec.ignore in this PR. The new work introduces a contentConfigChecksum field in the API, which includes the ignore value and many other fields that contribute to a new artifact to be built https://github.com/fluxcd/source-controller/compare/gitrepo-rec-fixes?expand=1#diff-4472354d9c6813ee6537b2625d7aa7eef2522f48bc45548c1397f207ff85cbc9R214-R223 .
I'd recommend to wait for that change to be finalized and merged first, and then based on those changes, add a similar change for the Bucket reconciler for detecting ignore + all other config changes.
Sorry for this. It wasn't planned before but the solution to fixing other stuff covers the issue that this PR tries to fix.

darkowlzz · 2022-06-06T16:50:48Z

@angelokurtis flux v0.31.0 was just released with the aforementioned improvements in the GitRepo reconciler. Your initial issue #704 is closed. But you can still add a similar change in the Bucket reconciler based on the GitRepo reconcile change in 581695b. We can introduce the same field contentConfigChecksum in the status of a Bucket object and use it to detect changes. This may also result in restructuring some parts of the Bucket reconciler, but I think that's okay. Feel free to ask any questions about it that you may have.

stefanprodan · 2022-10-11T09:09:17Z

Superseded by #926

angelokurtis mentioned this pull request May 6, 2022

Updating the spec.ignore field of a GitRepository has no effect #704

Closed

1 task

angelokurtis force-pushed the fix-excluded-patterns branch from d3d1482 to b30bc0c Compare May 9, 2022 19:11

darkowlzz reviewed May 10, 2022

View reviewed changes

api/v1beta2/bucket_types.go Outdated Show resolved Hide resolved

api/v1beta2/bucket_types.go Outdated Show resolved Hide resolved

controllers/gitrepository_controller.go Outdated Show resolved Hide resolved

angelokurtis force-pushed the fix-excluded-patterns branch 2 times, most recently from 03b53dd to 8f4462f Compare May 10, 2022 21:10

darkowlzz reviewed May 11, 2022

View reviewed changes

darkowlzz added enhancement New feature or request area/docs Documentation related issues and pull requests labels May 11, 2022

hiddeco reviewed May 11, 2022

View reviewed changes

hiddeco added area/git Git related issues and pull requests area/bucket Bucket related issues and pull requests labels May 11, 2022

angelokurtis added 2 commits May 13, 2022 09:02

Update artifact tarball when spec.ignore changes

9009c39

This commit adds a new field `status.ignoreChecksum` which is used to detect changes on `spec.ignore`, that controls the exclusions for the object's `status.artifact` Signed-off-by: Tiago Angelo <kurtis.angelo@gmail.com>

Testing spec ignore update scenarios

6208eff

Signed-off-by: Tiago Angelo <kurtis.angelo@gmail.com>

angelokurtis force-pushed the fix-excluded-patterns branch from 8f4462f to 6208eff Compare May 13, 2022 18:21

angelokurtis added 2 commits May 13, 2022 15:26

Doc adjustments

6426893

Signed-off-by: Tiago Angelo <kurtis.angelo@gmail.com>

Sorting imports

92a666b

Signed-off-by: Tiago Angelo <kurtis.angelo@gmail.com>

hiddeco reviewed May 13, 2022

View reviewed changes

Documenting new ignoreChecksum status fields

1e8980a

Signed-off-by: Tiago Angelo <kurtis.angelo@gmail.com>

angelokurtis requested review from darkowlzz and hiddeco May 17, 2022 19:52

hiddeco reviewed May 18, 2022

View reviewed changes

stefanprodan mentioned this pull request Oct 10, 2022

Add explicit observed artifact content configurations in status #926

Merged

stefanprodan closed this Oct 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update artifact tarball when `spec.ignore` changes #712

Update artifact tarball when `spec.ignore` changes #712

angelokurtis commented May 6, 2022

darkowlzz commented May 9, 2022

hiddeco commented May 9, 2022

angelokurtis commented May 9, 2022

darkowlzz left a comment •

edited

Loading

stefanprodan commented May 10, 2022

darkowlzz May 11, 2022 •

edited

Loading

angelokurtis May 13, 2022

darkowlzz May 11, 2022

angelokurtis May 13, 2022

darkowlzz commented May 11, 2022

hiddeco May 11, 2022

angelokurtis May 13, 2022

hiddeco May 11, 2022

angelokurtis May 13, 2022

hiddeco May 11, 2022

angelokurtis May 13, 2022

hiddeco left a comment

angelokurtis commented May 13, 2022

hiddeco May 18, 2022

hiddeco May 18, 2022

darkowlzz commented May 18, 2022

darkowlzz commented Jun 6, 2022 •

edited

Loading

stefanprodan commented Oct 11, 2022

	// Ensure .spec.ignore checksum is up-to-date
	// Ensure .status.ignoreChecksum is up-to-date with .spec.ignore.

	// It is formatted as `<algo>:<checksum>`. For example: `sha256:<checksum>`.
	// It has the format of `<algo>:<checksum>`, for example: `sha256:<checksum>`.

Update artifact tarball when spec.ignore changes #712

Update artifact tarball when spec.ignore changes #712

Conversation

angelokurtis commented May 6, 2022

darkowlzz commented May 9, 2022

hiddeco commented May 9, 2022

angelokurtis commented May 9, 2022

darkowlzz left a comment • edited Loading

Choose a reason for hiding this comment

stefanprodan commented May 10, 2022

darkowlzz May 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkowlzz commented May 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiddeco left a comment

Choose a reason for hiding this comment

angelokurtis commented May 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkowlzz commented May 18, 2022

darkowlzz commented Jun 6, 2022 • edited Loading

stefanprodan commented Oct 11, 2022

Update artifact tarball when `spec.ignore` changes #712

Update artifact tarball when `spec.ignore` changes #712

darkowlzz left a comment •

edited

Loading

darkowlzz May 11, 2022 •

edited

Loading

darkowlzz commented Jun 6, 2022 •

edited

Loading