Drop support for series in bundles #17350
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jack-w-shaw
force-pushed
the
JUJU-5996_drop_support_for_series_in_bundles
branch
from
e166f3d
to
b4ffde6
Compare
jack-w-shaw
force-pushed
the
JUJU-5996_drop_support_for_series_in_bundles
branch
2 times, most recently
from
3d366da
to
4f0ca93
Compare
jack-w-shaw
force-pushed
the
JUJU-5996_drop_support_for_series_in_bundles
branch
from
4f0ca93
to
cec4f30
Compare
jack-w-shaw
force-pushed
the
JUJU-5996_drop_support_for_series_in_bundles
branch
from
cec4f30
to
4ea92d6
Compare
We have discussed this together further and landed on erroring out if series is provided, but parsing for this in |
jack-w-shaw
force-pushed
the
JUJU-5996_drop_support_for_series_in_bundles
branch
from
4ea92d6
to
a47fb94
Compare
| errs := []string{} | ||
|
|
||
| // The first dslist is always the bundle itself | ||
| bundleErrs := verifyBundleNoSeries(dslist[0].RawBundle()) |
There was a problem hiding this comment.
I know that there is a check above where this is called to check that dslist is empty, but I don't like this coding style. Instead, can this return an empty string if the list is empty.
There was a problem hiding this comment.
You mean like a catch guard? Sure thing
| } else if err != nil { | ||
| // The bundle should already have been parsed if we're | ||
| // calling this check, so we should not see this | ||
| return []string{fmt.Sprintf("programming error: %w", err)} |
There was a problem hiding this comment.
Remove programming error. I'd just return the error, things can be refactored and seeing programming error is not ideal.
jack-w-shaw
force-pushed
the
JUJU-5996_drop_support_for_series_in_bundles
branch
from
a47fb94
to
072cf14
Compare
jujubot
added a commit
to juju/charm
that referenced
this pull request
May 21, 2024
…eive_raw_bundle #427 Add this RawBundle method to Bundle interface and BundleDataSource In Juju, we have come across a need to run some extra parsing + verification which we do not want to do in juju/charm. We want to keep the data model clean in juju/charm. As such, we need to way to keep the raw bundle data around for later parsing Add this to BundleDataSource, as this is what we use in Juju However, we also implement our own BundleDataSource in Juju. built with the Bundle interface here. So we need to add this method to Bundle as well You can see a draft implementation for this change here: juju/juju#17350 As a flyby, delete some incorrect comments ## QA Steps Compile into juju form the following PR and run it's QA steps: juju/juju#17350
jack-w-shaw
force-pushed
the
JUJU-5996_drop_support_for_series_in_bundles
branch
from
072cf14
to
e1f3aea
Compare
SimonRichardson
approved these changes
May 21, 2024
Support for series in bundles is being dropped in 4.0 (major version bump). Error out if bundles include series This means we no longer need to parse series into base in the rest of our bundle deploy code We do not want to rely the juju/charm BundleData struct to parse series. We want to keep this data structure clean, so will be removing from BundleData very soon Series As such, we need to parse the bundle ourselves. This requires using a new miner version of "juju/charm" which allows us to access the raw unparsed bundle Also, swap our series for base in test bundles
jack-w-shaw
force-pushed
the
JUJU-5996_drop_support_for_series_in_bundles
branch
from
e1f3aea
to
d74fefb
Compare
|
/merge |
1 similar comment
|
/merge |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Drop support for series in bundles
Support for series in bundles is being dropped in 4.0 (major version
bump). Error out if bundles include series
This means we no longer need to parse series into base in the rest of
our bundle deploy code
We do not want to rely the juju/charm BundleData struct to parse series.
We want to keep this data structure clean, so will be removing from
BundleData very soon Series
As such, we need to parse the bundle ourselves. This requires using a
new miner version of "juju/charm" which allows us to access the raw
unparsed bundle
We can also drop compatibility parsing in bundle handlers
Also, rename
checkExplicitSeriestocheckExplicitBaseand stop checkingif
seriesis provided here.Also, swap our series for base in test bundles
Checklist
QA steps
Attempt to deploy some bundles
(Note: charmed-kubernetes rev 1243 contains series on the top level bundle, no where else)
Attempt to deploy some bundles with overlays
Links
Jira card: JUJU-5996