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.

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

fix(sync): allow local paths in vendir config #191

Merged
merged 12 commits into from
Jan 1, 2024
Merged

fix(sync): allow local paths in vendir config #191

merged 12 commits into from
Jan 1, 2024

Conversation

Zebradil
Copy link
Member

@Zebradil Zebradil commented Dec 26, 2023 •
edited
Loading

Fixes #189

@Zebradil Zebradil linked an issue Dec 26, 2023 that may be closed by this pull request
[bug] directory source in vendir config is broken #189
Closed
@Zebradil
Copy link
Member Author

Zebradil commented Dec 26, 2023 •
edited
Loading

This is currently not working due to a vendir bug: multiple vendir processes can't run from the same directory because they use the same .vendir-tmp directory.

A solution is proposed in carvel-dev/vendir#345

kbudde
kbudde requested changes Dec 27, 2023
View reviewed changes
Copy link
Member

@kbudde kbudde left a comment

Choose a reason for hiding this comment

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

What's the reason of the failing test?

@fritzduchardt
Copy link
Collaborator

fritzduchardt commented Dec 28, 2023 •
edited
Loading

This is currently not working due to a vendir bug: multiple vendir processes can't run from the same directory because they use the same .vendir-tmp directory.

A solution is proposed in carvel-dev/vendir#345

Nicely done. I wonder whether this might have been the issue with struggled to reproduce when we migrated iam to myks.. Never mind, back then we were using separate vendir output folders.

fritzduchardt
fritzduchardt requested changes Dec 28, 2023
View reviewed changes
Copy link
Collaborator

@fritzduchardt fritzduchardt left a comment

Choose a reason for hiding this comment

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

Looks good and does work for me. Left a minor comment about the logging. Thanks for the quick implementation. Will start using this as soon as it is out.

Out of curiosity: did the discussed solution with the vendir directory flag not work for you factually or you just liked the ytt rewrite better?

internal/myks/sync.go Outdated Show resolved Hide resolved
@Zebradil
Copy link
Member Author

@fritzduchardt

did the discussed solution with the vendir directory flag not work for you factually or you just liked the ytt rewrite better?

To be honest, I forgot about this option. But anyway, it seems to me a bit hacky. With the overlay, I don't care about what's inside the vendir config, execute vendir sync once and do not need additional arguments.

@Zebradil Zebradil mentioned this pull request Dec 29, 2023
Closed
@kbudde kbudde mentioned this pull request Dec 30, 2023
Merged
@kbudde
Copy link
Member

kbudde commented Jan 1, 2024

@Zebradil if you merge/rebase dev (with #199), the tests should succeed.

@kbudde
Copy link
Member

kbudde commented Jan 1, 2024 •
edited
Loading

Ha, renovate was so "friendly" to update vendir to the latest main commit in #200.
Therefore it's not the needed branch anymore.

@Zebradil
Copy link
Member Author

Zebradil commented Jan 1, 2024

Strangely, I wasn't able to reproduce the issue with tests locally. But now it seems to reproduce.

@codecov Codecov
Copy link

codecov bot commented Jan 1, 2024

Codecov Report

Attention: 50 lines in your changes are missing coverage. Please review.

Comparison is base (053e89f) 47.54% compared to head (79a0f6f) 46.65%.
Report is 5 commits behind head on dev.

Files Patch % Lines
cmd/vendir/vendir.go 0.00% 33 Missing ⚠️
cmd/cleanup.go 0.00% 12 Missing ⚠️
internal/myks/sync.go 82.35% 1 Missing and 2 partials ⚠️
cmd/root.go 0.00% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##              dev     #191      +/-   ##
==========================================
- Coverage   47.54%   46.65%   -0.90%     
==========================================
  Files          26       28       +2     
  Lines        2509     2542      +33     
==========================================
- Hits         1193     1186       -7     
- Misses       1131     1174      +43     
+ Partials      185      182       -3

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

@Zebradil Zebradil merged commit 73233eb into dev Jan 1, 2024
5 checks passed
@Zebradil Zebradil deleted the 189-bug-directory-source-in-vendir-config-is-broken branch January 1, 2024 12:19
mykso-bot added a commit that referenced this pull request Jan 7, 2024
@mykso-bot
chore(release): 3.2.0 [skip ci]
0bb0d18 
# [3.2.0](v3.1.0...v3.2.0) (2024-01-07)

### Bug Fixes

* **deps:** update github.com/cppforlife/go-cli-ui digest to 9954948 ([#201](#201)) ([9836249](9836249))
* **deps:** update golang.org/x/exp digest to be819d1 ([#204](#204)) ([9928ad1](9928ad1))
* **deps:** update module golang.org/x/sync to v0.6.0 ([#205](#205)) ([d3b8ea0](d3b8ea0))
* **deps:** update module golang.org/x/term to v0.16.0 ([#206](#206)) ([002cfe0](002cfe0))
* **sync:** allow local paths in vendir config ([#191](#191)) ([73233eb](73233eb))

### Features

* **cleanup:** added dedicated command ([#198](#198)) ([48fa589](48fa589)), closes [#130](#130)
* **vendir:** embed vendir into myks ([#199](#199)) ([95ecfa8](95ecfa8))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fritzduchardt fritzduchardt fritzduchardt requested changes

@kbudde kbudde kbudde approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[bug] directory source in vendir config is broken
3 participants
@Zebradil @fritzduchardt @kbudde