Skip to content

Commit

Permalink
Merge pull request juju#16561 from jack-w-shaw/remove_series_equals_b…
Browse files Browse the repository at this point in the history
…undle_checks

juju#16561

This is legacy code from the charmstore days. Now we no longer use charmstore, it is impossible (and not supported) for charm urls to have "bundle" in the series field

In charmhub, we designate entities to be bundles using the charm origin 'type' field. We can just check this to determine if an entity is a bundle

As a drive-by, refactor relevant some tests to remove a redundant method, and fill out some fakes more fully

## Checklist

- [x] Code style: imports ordered, good names, simple structure, etc
- [x] Comments saying why design decisions were made
- [x] Go unit tests, with comments saying what you're testing
- ~[ ] [Integration tests](https://github.com/juju/juju/tree/main/tests), with comments saying what you're testing~
- [ ] [doc.go](https://discourse.charmhub.io/t/readme-in-packages/451) added or updated in changed packages

## QA steps

```
$ cat bun.yaml
applications:
 ubuntu:
 charm: ubuntu
$ juju deploy ./bun.yaml
Located charm "ubuntu" in charm-hub, channel stable
Executing changes:
- upload charm ubuntu from charm-hub with architecture=amd64
- deploy application ubuntu from charm-hub
Deploy of bundle completed.
```

```
$ cat bun.yaml
applications:
 ubuntu:
 charm: cos-lite
Located charm "cos-lite" in charm-hub, channel stable
ERROR cannot deploy bundle: expected charm, got bundle "cos-lite"
```

```
$ juju deploy juju-qa-bundle-test
Located bundle "juju-qa-bundle-test" in charm-hub, revision 3
Located charm "juju-qa-test" in charm-hub, channel 2.0/stable
Located charm "juju-qa-test" in charm-hub, channel latest/candidate
Located charm "ntp" in charm-hub, channel stable
Located charm "ntp" in charm-hub, channel stable
Executing changes:
- upload charm juju-qa-test from charm-hub for base [email protected]/stable from channel 2.0/stable with architecture=amd64
- deploy application juju-qa-test from charm-hub on [email protected]/stable with 2.0/stable
 added resource foo-file
- upload charm juju-qa-test from charm-hub for base [email protected]/stable from channel latest/candidate with architecture=amd64
- deploy application juju-qa-test-focal from charm-hub on [email protected]/stable with latest/candidate using juju-qa-test
 added resource foo-file
- upload charm ntp from charm-hub for base [email protected]/stable from channel stable with architecture=amd64
- deploy application ntp from charm-hub on [email protected]/stable with stable
- upload charm ntp from charm-hub for base [email protected]/stable from channel stable with architecture=amd64
- deploy application ntp-focal from charm-hub on [email protected]/stable with stable using ntp
- add new machine 0
- add new machine 1
- add relation ntp:juju-info - juju-qa-test:juju-info
- add relation ntp-focal:juju-info - juju-qa-test-focal:juju-info
- add unit juju-qa-test/0 to new machine 0
- add unit juju-qa-test-focal/0 to new machine 1
Deploy of bundle completed.
```

```
$ juju charm-resources cos-lite
No resources to display.
```
  • Loading branch information
jujubot authored Nov 17, 2023
2 parents 34225ff + b37d915 commit 3bbcf81
Show file tree
Hide file tree
Showing 5 changed files with 24 additions and 16 deletions.
4 changes: 2 additions & 2 deletions cmd/juju/application/deployer/bundlehandler.go
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,7 @@ func (h *bundleHandler) resolveCharmsAndEndpoints() error {
}

h.ctx.Infof(formatLocatedText(ch, origin))
if url.Series == "bundle" || origin.Type == "bundle" {
if origin.Type == "bundle" {
return errors.Errorf("expected charm, got bundle %q", ch.Name)
}

Expand Down Expand Up @@ -648,7 +648,7 @@ func (h *bundleHandler) addCharm(change *bundlechanges.AddCharmChange) error {
if err != nil {
return errors.Annotatef(err, "cannot resolve %q", ch.Name)
}
if url.Series == "bundle" || resolvedOrigin.Type == "bundle" {
if resolvedOrigin.Type == "bundle" {
return errors.Errorf("expected charm, got bundle %q %v", ch.Name, resolvedOrigin)
}
workloadBases, err := SupportedJujuBases(jujuclock.WallClock.Now(), base, h.modelConfig.ImageStream())
Expand Down
6 changes: 1 addition & 5 deletions cmd/juju/application/store/charmadapter.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,14 +85,10 @@ func (c *CharmAdaptor) ResolveBundleURL(maybeBundle *charm.URL, preferredOrigin
return nil, commoncharm.Origin{}, errors.Trace(err)
}
// We're a bundle so return out before handling the invalid flow.
if transport.BundleType.Matches(origin.Type) || storeCharmOrBundleURL.Series == "bundle" {
if transport.BundleType.Matches(origin.Type) {
return storeCharmOrBundleURL, origin, nil
}

logger.Debugf(
`cannot interpret as charmstore bundle: %v (series) != "bundle"`,
storeCharmOrBundleURL.Series,
)
return nil, commoncharm.Origin{}, errors.NotValidf("charmstore bundle %q", maybeBundle)
}

Expand Down
23 changes: 21 additions & 2 deletions cmd/juju/application/store/charmadapter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -90,12 +90,12 @@ func (s *resolveSuite) TestResolveBundle(c *gc.C) {

curl, err := charm.ParseURL("ch:testme")
c.Assert(err, jc.ErrorIsNil)
s.expectCharmResolutionCall(curl, "edge", nil)
s.expectBundleResolutionCall(curl, "edge", nil)

curl.Series = "bundle"
origin := commoncharm.Origin{
Source: commoncharm.OriginCharmHub,
Risk: "edge",
Type: "bundle",
}
charmAdapter := store.NewCharmAdaptor(s.charmsAPI, func() (store.DownloadBundleClient, error) {
return s.downloadClient, nil
Expand Down Expand Up @@ -154,10 +154,28 @@ func (s *resolveSuite) setupMocks(c *gc.C) *gomock.Controller {
return ctrl
}

func (s *resolveSuite) expectBundleResolutionCall(curl *charm.URL, out string, err error) {
origin := commoncharm.Origin{
Source: commoncharm.OriginCharmHub,
Risk: out,
Type: "bundle",
}
retVal := []apicharm.ResolvedCharm{{
URL: curl,
Origin: origin,
SupportedBases: []base.Base{
base.MustParseBaseFromString("[email protected]"),
base.MustParseBaseFromString("[email protected]"),
},
}}
s.charmsAPI.EXPECT().ResolveCharms(gomock.Any()).Return(retVal, err)
}

func (s *resolveSuite) expectCharmResolutionCall(curl *charm.URL, out string, err error) {
origin := commoncharm.Origin{
Source: commoncharm.OriginCharmHub,
Risk: out,
Type: "charm",
}
retVal := []apicharm.ResolvedCharm{{
URL: curl,
Expand All @@ -174,6 +192,7 @@ func (s *resolveSuite) expectCharmResolutionCallWithAPIError(curl *charm.URL, ou
origin := commoncharm.Origin{
Source: commoncharm.OriginCharmHub,
Risk: out,
Type: "charm",
}
retVal := []apicharm.ResolvedCharm{{
URL: curl,
Expand Down
3 changes: 0 additions & 3 deletions cmd/juju/application/store/resolve.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,8 @@ package store
import (
"github.com/juju/charm/v11"
"github.com/juju/errors"
"github.com/juju/loggo"
)

var logger = loggo.GetLogger("juju.cmd.juju.application.store")

// ResolvedBundle decorates a charm.Bundle instance with a type that implements
// the charm.BundleDataSource interface.
type ResolvedBundle struct {
Expand Down
4 changes: 0 additions & 4 deletions cmd/juju/resource/charmresources.go
Original file line number Diff line number Diff line change
Expand Up @@ -228,10 +228,6 @@ func resolveCharm(raw string) (*charm.URL, error) {
if err != nil {
return nil, errors.Trace(err)
}

if charmURL.Series == "bundle" {
return nil, errors.NotSupportedf("charm bundles")
}
if !charm.CharmHub.Matches(charmURL.Schema) {
return nil, errors.BadRequestf("only supported with charmhub charms")
}
Expand Down

0 comments on commit 3bbcf81

Please sign in to comment.