Skip to content

Conversation

crossy-l
Copy link

@crossy-l crossy-l commented Jun 16, 2025

In https://github.com/orgs/flipt-io/discussions/4311, the audit logs were already improved. Now this is a small follow up.
I want to add the id's of the rollouts/segments to the flipt export. This way, when an audit log comes in, the previous version of that record can easily be looked up using it's id in a previous export.

This PR simply adds the ids to the export interfaces.

The tests still need to be updated however. That is of course, if you agree with the changes.

Also this is my first ever open source PR :)

@crossy-l crossy-l requested a review from a team as a code owner June 16, 2025 13:24
@dosubot dosubot bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Jun 16, 2025
Copy link
Collaborator

@markphelps markphelps left a comment

Choose a reason for hiding this comment

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

thanks @crossy-l , looks good to me to continue

I think we should also bump the version here to 1.5 too , even though its a nonbreaking change

latestVersion = v1_4

@markphelps
Copy link
Collaborator

congrats on the your first open source contribution @crossy-l !! thank you!

@crossy-l
Copy link
Author

Thanks. I will look into the test cases and bump the version in the following days.

@crossy-l
Copy link
Author

crossy-l commented Jun 20, 2025

@markphelps all right, now trying to manually update all test cases seems incredibly tedious. I put a little snippet inside the exporter_test to generate new fixtures like so (this is only a small demonstration, not that I actually want that inside the exporter_test):

t.Run(fmt.Sprintf("%s (%s)", tc.name, ext), func(t *testing.T) {
				var (
					exporter = NewExporter(tc.lister, tc.namespaces, tc.allNamespaces, tc.sortByKey)
					b        = new(bytes.Buffer)
				)

				err := exporter.Export(context.Background(), ext, b)
				require.NoError(t, err)

// - snippet here
				// Overwrite existing fixture file
				outputPath := tc.path + "." + string(ext)
				err = os.WriteFile(outputPath, b.Bytes(), 0644)
				require.NoError(t, err)
				b = bytes.NewBuffer(b.Bytes())
// - end of snippet
				in, err := os.ReadFile(tc.path + "." + string(ext))
				require.NoError(t, err)

				var (
					expected = ext.NewDecoder(bytes.NewReader(in))
					found    = ext.NewDecoder(b)
				)

				// handle newline delimited JSON
				for {
					var exp, fnd any
					eerr := expected.Decode(&exp)
					ferr := found.Decode(&fnd)
					require.Equal(t, eerr, ferr)

					if errors.Is(ferr, io.EOF) {
						break
					}
					require.NoError(t, ferr)

					assert.Equal(t, exp, fnd)
				}
			})

This however introduces lot's of small formatting differences between the existing fixtures.
I am wondering if you would also want some kind of command for generating fixtures based on the expected test. Of course these generated fixtures would need to be manually checked. However when changing something in the export functionality it will be trivial to update tests.

I will just commit the generated fixtures so you can see the differences.
(And sorry for the force push on the branch, I committed with the wrong config.)

@dosubot dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Jun 20, 2025
@crossy-l crossy-l force-pushed the lk/fix-flipt-export-ids branch from 41a7b86 to a895cb8 Compare June 20, 2025 14:33
Copy link

codecov bot commented Jun 20, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 63.87%. Comparing base (563759a) to head (12a61da).
⚠️ Report is 11 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4359   +/-   ##
=======================================
  Coverage   63.87%   63.87%           
=======================================
  Files         172      172           
  Lines       17659    17661    +2     
=======================================
+ Hits        11279    11281    +2     
  Misses       5705     5705           
  Partials      675      675           
Flag Coverage Δ
unittests 63.87% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@markphelps
Copy link
Collaborator

@crossy-l dont worry about the formatting I'll run our formatter prettier on those files

@crossy-l
Copy link
Author

Hello,
is this done on my part? The integration tests seem to pass at least when I tried it locally.
Can this be merged? @markphelps

@markphelps markphelps added the automerge Used by Kodiak bot to automerge PRs label Aug 28, 2025
@markphelps
Copy link
Collaborator

sorry for the delay @crossy-l !! thank you for this contribution!

markphelps pushed a commit that referenced this pull request Sep 6, 2025
The PR #4359 adds ID fields to rollouts and constraints in the export.
This change updates the tests to handle these new fields properly:

1. CLI tests now check for individual fields rather than exact YAML blocks
   since IDs are included between the fields
2. Integration tests normalize YAMLs by removing IDs before comparison
   since IDs are server-generated and not stable across import/export

This preserves the PR's intent (adding IDs to exports) while fixing tests.
markphelps pushed a commit to crossy-l/flipt that referenced this pull request Sep 6, 2025
The PR flipt-io#4359 adds ID fields to rollouts and constraints in the export.
This change updates the tests to handle these new fields properly:

1. CLI tests now check for individual fields rather than exact YAML blocks
   since IDs are included between the fields
2. Integration tests normalize YAMLs by removing IDs before comparison
   since IDs are server-generated and not stable across import/export

This preserves the PR's intent (adding IDs to exports) while fixing tests.

Signed-off-by: Claude <[email protected]>
@markphelps markphelps force-pushed the lk/fix-flipt-export-ids branch from 8295df0 to 66959b6 Compare September 6, 2025 16:44
markphelps pushed a commit to crossy-l/flipt that referenced this pull request Sep 6, 2025
The PR flipt-io#4359 adds ID fields to rollouts and constraints in the export.
This change updates the tests to handle these new fields properly:

1. CLI tests now check for individual fields rather than exact YAML blocks
   since IDs are included between the fields
2. Integration tests normalize YAMLs by removing IDs before comparison
   since IDs are server-generated and not stable across import/export

This preserves the PR's intent (adding IDs to exports) while fixing tests.

Signed-off-by: markphelps <[email protected]>
Signed-off-by: Claude <[email protected]>
@markphelps markphelps force-pushed the lk/fix-flipt-export-ids branch 2 times, most recently from e04d887 to 05fb6b4 Compare September 6, 2025 17:15
@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:XXL This PR changes 1000+ lines, ignoring generated files. labels Sep 6, 2025
@markphelps markphelps force-pushed the lk/fix-flipt-export-ids branch from 05fb6b4 to d8bab25 Compare September 6, 2025 17:19
markphelps pushed a commit to crossy-l/flipt that referenced this pull request Sep 6, 2025
The PR flipt-io#4359 adds ID fields to rollouts and constraints in the export.
This change updates the tests to handle these new fields properly:

1. CLI tests now check for individual fields rather than exact YAML blocks
   since IDs are included between the fields
2. Integration tests strip ID fields using regex before comparison
   since IDs are server-generated UUIDs and not stable across import/export

This preserves the PR's intent (adding IDs to exports) while fixing tests.

Signed-off-by: markphelps <[email protected]>
@markphelps markphelps force-pushed the lk/fix-flipt-export-ids branch from d8bab25 to e15de90 Compare September 6, 2025 17:20
The PR flipt-io#4359 adds ID fields to rollouts and constraints in the export.
This change updates the tests to handle these new fields properly:

1. CLI tests now check for individual fields rather than exact YAML blocks
   since IDs are included between the fields
2. Integration tests strip ID fields using regex before comparison
   since IDs are server-generated UUIDs and not stable across import/export

This preserves the PR's intent (adding IDs to exports) while fixing tests.

Signed-off-by: markphelps <[email protected]>
@markphelps markphelps force-pushed the lk/fix-flipt-export-ids branch from e15de90 to 12a61da Compare September 6, 2025 17:37
@kodiakhq kodiakhq bot removed the automerge Used by Kodiak bot to automerge PRs label Sep 10, 2025
Copy link
Contributor

kodiakhq bot commented Sep 10, 2025

This PR currently has a merge conflict. Please resolve this and then re-add the automerge label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:XL This PR changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants