Make multi-type annotation settings match docs #2522
Conversation
|
I get the same test failures locally with and without this change - I think these must be my setup rather than an actual problem. Hopefully the validation will be green but I'll mark as draft again if not. |
treuherz
force-pushed
the
annotation-per-type
branch
from
2c77cd9
to
6e6f4c2
Compare
|
A dep changed from |
util/buildflags/export_test.go
Outdated
|
|
||
| for _, test := range tests { | ||
| t.Run(test.name, func(t *testing.T) { | ||
| got, gotErr := ParseAnnotations(test.in) |
util/buildflags/export_test.go
Outdated
| if test.wantErr != "" { | ||
| require.ErrorContains(t, gotErr, test.wantErr) | ||
| } else { | ||
| assert.NoError(t, gotErr) |
util/buildflags/export.go
Outdated
| @@ -81,7 +81,8 @@ func ParseExports(inp []string) ([]*controllerapi.ExportEntry, error) { | |||
|
|
|||
| func ParseAnnotations(inp []string) (map[exptypes.AnnotationKey]string, error) { | |||
| // TODO: use buildkit's annotation parser once it supports setting custom prefix and ":" separator | |||
| annotationRegexp := regexp.MustCompile(`^(?:([a-z-]+)(?:\[([A-Za-z0-9_/-]+)\])?:)?(\S+)$`) | |||
| annotationRegexp := regexp.MustCompile(`^((?:[a-z-]+(?:\[[A-Za-z0-9_/-]+\])?)(?:,[a-z-]+(?:\[[A-Za-z0-9_/-]+\])?)*:)?(\S+)$`) | |||
There was a problem hiding this comment.
This needs comments about why there are multiple and what's the difference
There was a problem hiding this comment.
The regexp is pretty inscrutable, I might try breaking it up into cuts and splits and just using a smaller regexp for pulling out the platform specifier, if it’s all the same to you
util/buildflags/export_test.go
Outdated
| slices.SortFunc(wantKVs, sortFunc) | ||
| slices.SortFunc(gotKVs, sortFunc) | ||
|
|
||
| if diff := gocmp.Diff(wantKVs, gotKVs); diff != "" { |
There was a problem hiding this comment.
I think you can just use assert.Equal in here. No need for gocmp.
| assert.NoError(t, gotErr) | ||
| } | ||
|
|
||
| // Can't compare maps with pointer in their keys, need to extract and sort the map entries |
There was a problem hiding this comment.
might make sense to make this covert code into a function and then convert both maps. Atm there is duplication where exactly the same commands need to run on both maps.
The Docker docs in multiple places describe passing an annotation at the command line like "index,manifest:com.example.name=my-cool-image", and say that this will result in the annotation being applied to both the index and the manifest. It doesn't seem like this was actually implemented, and instead it just results in an annotation key with "index,manifest:" at the beginning being applied to the manifest. This change splits the part of the key before the colon by comma, and creates an annotation for each type/platform given, so the implementation should now match the docs. Signed-off-by: Eli Treuherz <[email protected]>
Signed-off-by: Eli Treuherz <[email protected]>
Signed-off-by: Eli Treuherz <[email protected]>
treuherz
force-pushed
the
annotation-per-type
branch
from
41431d8
to
3d0951b
Compare
The Docker docs in multiple places describe passing an annotation at the command line like "index,manifest:com.example.name=my-cool-image", and say that this will result in the annotation being applied to both the index and the manifest. It doesn't seem like this was actually implemented, and instead it just results in an annotation key with "index,manifest:" at the beginning being applied to the manifest.
This change splits the part of the key before the colon by comma, and creates an annotation for each type/platform given, so the implementation should now match the docs.