Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Improve bind imports, support lower case 'source' in yaml, add go test #517

Merged
merged 1 commit into from
Oct 3, 2023

Conversation

smoser
Copy link
Contributor

@smoser smoser commented Oct 2, 2023

Entries stacker.yaml's "binds" are of type Bind.
When imported from yaml, support was present for these three things:

  1. ascii art specifying source and dest ("source -> dest")
  2. source only ("source")
  3. map with 'Source' and 'Dest'.

Support for '3' is dropped, and replaced by map with 'source' and 'dest'. 'Source' and 'Dest' are inconsistent with the rest of stacker.yaml. Note that importing from json still does support capital letter variants as the golang json library is case insensitive.

Improvements to the bind yaml and json support generally rely more heavily on the library to do the lifting.

What type of PR is this?

Which issue does this PR fix:

What does this PR do / Why do we need it:

If an issue # is not available please add repro steps and logs showing the issue:

Testing done on this change:

Automation added to e2e:

Will this break upgrades or downgrades?

Does this PR introduce any user-facing change?:


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@smoser smoser requested review from rchincha and hallyn as code owners October 2, 2023 20:06
@smoser smoser force-pushed the fix/lower-case-bind-source branch from 0a8f62e to be4532d Compare October 2, 2023 20:07
@smoser
Copy link
Contributor Author

smoser commented Oct 3, 2023

builds failed, like below. I uploaded #518 and I'm hitting rebuids.

Couldn't obtain file info of https://github.com/lvmteam/lvm2/archive/refs/tags/v2_03_18.tar.gz, using cached copy
overlay-dirs, possibly modified after import: []
loading docker://alpine:edge
error: initializing source docker://alpine:edge: pinging container registry registry-1.docker.io: received unexpected HTTP status: 503 Service Unavailable
couldn't import base layer alpine 

@smoser smoser force-pushed the fix/lower-case-bind-source branch 2 times, most recently from ae64b1b to 49cdd1a Compare October 3, 2023 02:03
…o test

Entries stacker.yaml's "binds" are of type Bind.
When imported from yaml, support was present for these three things:
1. ascii art specifying source and dest ("source -> dest")
2. source only ("source")
3. map with 'Source' and 'Dest'.

Support for '3' is dropped, and replaced by map with 'source' and
'dest'.  'Source' and 'Dest' are inconsistent with the rest of stacker.yaml.
Note that importing from json still does support capital letter variants
as the golang json library is case insensitive.

Improvements to the bind yaml and json support generally rely more
heavily on the library to do the lifting.

Signed-off-by: Scott Moser <[email protected]>
@smoser smoser force-pushed the fix/lower-case-bind-source branch from 49cdd1a to fe83356 Compare October 3, 2023 12:26
@codecov
Copy link

codecov bot commented Oct 3, 2023

Codecov Report

Merging #517 (fe83356) into main (96dbbe5) will increase coverage by 0.62%.
The diff coverage is 70.73%.

@@            Coverage Diff             @@
##             main     #517      +/-   ##
==========================================
+ Coverage   13.14%   13.77%   +0.62%     
==========================================
  Files          40       40              
  Lines        5712     5671      -41     
==========================================
+ Hits          751      781      +30     
+ Misses       4837     4762      -75     
- Partials      124      128       +4     
Files Coverage Δ
pkg/types/layer.go 50.68% <70.73%> (+12.50%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@smoser smoser merged commit 4689ad5 into project-stacker:main Oct 3, 2023
9 checks passed
Sign up for free to 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