Skip to content

switch to gopkg.in/yaml.v3 #4178

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

switch to gopkg.in/yaml.v3 #4178

wants to merge 3 commits into from

Conversation

thaJeztah
Copy link
Member

Looks like most other projects switched over to v3, so let's follow suit.

@thaJeztah thaJeztah added area/config Related to registry config dependencies Pull requests that update a dependency file labels Dec 1, 2023
@thaJeztah
Copy link
Member Author

ah, dang; looks like this may need more work

@milosgajdos
Copy link
Member

This is failing e2e (and conformance) tests but passing unit tests 🤔

@thaJeztah
Copy link
Member Author

yup. initially I thought it was only used for config (so thought it would be relatively "safe" to update). Not sure what's causing it. At least may mean we're missing coverage somewhere

@Jamstah
Copy link
Collaborator

Jamstah commented Dec 1, 2023

There must be a reason they did a major bump :)

@thaJeztah
Copy link
Member Author

Yup; let me move this back to draft, and if I find some time, look what the breaking change is 😄

@thaJeztah thaJeztah marked this pull request as draft December 1, 2023 11:07
@milosgajdos
Copy link
Member

milosgajdos commented Dec 14, 2023 •
edited
Loading

I've rebased this PR @thaJeztah I might have some spare cycles to look into this tomorrow.

@thaJeztah @milosgajdos
switch to gopkg.in/yaml.v3
51002c6 
Looks like most other projects switched over to v3, so let's follow suit.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
Copy link
Member Author

Thanks; yes feel free to either carry / push to this PR, or close and open a replacement (don't think I'll have time myself) ❤️

@milosgajdos
fix: update the type switch when reading config variables
3f5546b 
gopkg.in/yaml.v3 changed the way it parses YAML.
See the following commit for epxplanation:
go-yaml/yaml@14f799f

Signed-off-by: Milos Gajdos <milosthegajdos@gmail.com>
@milosgajdos
fix: update code that switch types on invalid params
d467325 
Signed-off-by: Milos Gajdos <milosthegajdos@gmail.com>
@milosgajdos
Copy link
Member

milosgajdos commented Dec 14, 2023 •
edited
Loading

Thanks, @thaJeztah, I've found the cause. The new version of gopkg.in/yaml.v3 now parses generic k/v in the YAML differently. See this commit: go-yaml/yaml@14f799f

I am not entirely sure why the e2e / run-e2e-test-s3-storage workflow is failing because the tests pass on my laptop 🤔

E2E Tests 
$ make start-e2e-s3-env
docker compose -f tests/docker-compose-e2e-cloud-storage.yml up -d
[+] Building 0.0s (0/0)                                                                                                                                                                                                                                               docker:orbstack
[+] Running 4/4
 ✔ Container tests-minio-1       Healthy                                                                                                                                                                                                                                         0.0s
 ✔ Container tests-minio-init-1  Exited                                                                                                                                                                                                                                          0.0s
 ✔ Container tests-redis-1       Started                                                                                                                                                                                                                                         0.2s
 ✔ Container tests-registry-1    Started

$ bash ./tests/push.sh 127.0.0.0
200
latest: Pulling from library/hello-world
Digest: sha256:3155e04f30ad5e4629fac67d6789f8809d74fea22d4e9a82f757d28cee79e0c5
Status: Image is up to date for hello-world:latest
docker.io/library/hello-world:latest
The push refers to repository [127.0.0.0:5000/distribution/hello-world]
01bb4fce3eb1: Pushed
latest: digest: sha256:7e9b6e7ba2842c91cf49f3e214d04a7a496f8214356f41d81a6e6dcad11f11e3 size: 525
latest: Pulling from distribution/hello-world
Digest: sha256:7e9b6e7ba2842c91cf49f3e214d04a7a496f8214356f41d81a6e6dcad11f11e3
Status: Image is up to date for 127.0.0.0:5000/distribution/hello-world:latest
127.0.0.0:5000/distribution/hello-world:latest

@milosgajdos
Copy link
Member

I am not entirely sure why the e2e / run-e2e-test-s3-storage workflow is failing because the tests pass on my laptop 🤔

Ok, found the cause the tests fail, but I'm afraid the fix will require more config surgery. The unsafe config structs finally caught up with us 😄

@milosgajdos milosgajdos mentioned this pull request Dec 5, 2024
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
area/config Related to registry config dependencies Pull requests that update a dependency file
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@thaJeztah @milosgajdos @Jamstah