-
Notifications
You must be signed in to change notification settings - Fork 403
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
pkg/repo: inject chart metadata(name/version) into storage object #362
Conversation
f9f40d9
to
7201663
Compare
@scbizu This will fix the problem with strict semantic versioning deletion, but the bigger problem is why to allow a chart creation that cannot be deleted? just like 1.0-SNAPSHOT+PR-102 |
I think It's just a bug ? Or you ask why not use strict semantic versioning deletion before ? I don't know. Maybe chartmuseum itself allows the non-semantic charts ? |
In your code at https://github.com/helm/chartmuseum/pull/362/files#diff-e4a78696e7b46c8d2418e143d91eb293R119 |
XD sorry for my misunderstanding . I will take a look at this , maybe we can have a solution with this case , but I think it's better for you to use a valid strict version rather than the no strict one. |
I totally agree regarding having strict semantic versioning, was just wondering why fixing just the deletion of valid semver 2.0 and not fixing the general deletion bug :) |
The previous code here is to find the proper version to delete but has the bad version detection for some cases , so I replace this code with the semver version detection for better readability and understanding. (But I totally forget the non-strict semantic version :-| ) Maybe I should switch to another workaround. |
@scbizu - can we prefix this PR with "WIP"? |
7201663
to
e1af552
Compare
e1af552
to
7da6dde
Compare
@scbizu Any update on this issue? |
@kobynet just finish my side after store the chart version into Object metadata, and the PR is pending for merging chartmuseum/storage#51 If you are in hurry with this fix , could you self run the my patch , and check if it is ok :
If you need a test docker image please at me , I think I can provide one. |
That storage pr is now closed if you want to test here @scbizu |
OK, I see it. I will add some more tests for this PR , thank u . |
7da6dde
to
7199326
Compare
89273e5
to
99cfdc3
Compare
99cfdc3
to
9780110
Compare
Finally all green , do you have time do some code review for me ? @jdolitsky |
eacaffd
to
a00407c
Compare
…art version Signed-off-by: scnace <[email protected]>
32db8b7
to
46af326
Compare
4d0417b
to
cc62b99
Compare
cc62b99
to
24cfe64
Compare
…ame not found Signed-off-by: scnace <[email protected]>
24cfe64
to
24c2ee2
Compare
@jdolitsky hihi I re-edit the acceptance , now it seems right . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. Will release new storage module, then we will update and merge
Signed-off-by: scnace <[email protected]>
1ef6d5c
to
7982fa5
Compare
update the module dependency , but it seems like the dependabot will do it for me ? 🤔 |
@scbizu thanks, we can close the bot PRs if they are out of date now |
@scbizu did you guys solve this issue? I still encounter it.. That problem and also the fact that even when deleting charts that do not exist that return deleted true |
@pixelicous Did you bump to the latest version ? Or can you provide us something more details ? |
e.g.: 0.0.1-SNAPSHOT-1 , it is a valid semver format but cannot be parsed properly (returns chart version 1 rather than 0.0.1-SNAPSHOT-1 )
Fix #220
Remark
I think I should add some more annotations here:
Since deleting chart from index file should always delete the cache by chartname and chartversion , but the non-semver(or the chart not follow our parsing rules exactly) chart will be badly parsed the version with a incorrect number (as I comment in #220 (comment)) . To resolve this issue , we should keep this chart version metadata within the all cache lifecycle , more closely , for the cached chart , we should add the metadata just after the cache inited (and store it in the storage.StorageObject.Metadata) , if so , the
diff
from cache and server storage will carry with the chart version , and then theserver.regenerateRepositoryIndex
will correctly regenerate the latest state of the server storage in cache.