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.

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

Populate charm version for charms fetched from the charm store #9722

Merged
merged 2 commits into from
Feb 11, 2019

Conversation

achilleasa
Copy link
Contributor

@achilleasa achilleasa commented Feb 6, 2019

Description of change

When deploying local charms that contain a version file, the charm version details are updated with the contents of that file. This was not the case when attempting to deploy charms from the charm store which results in the version details not being included in the juju status output.

This PR bumps the sha for the charm.v6 dependency (see: juju/charm#269) which provides a helper charm archive method for fetching the version of a charm and updates the relevant code to populate the charm version field prior to persisting its details to the store.

QA steps

$ juju deploy vault
$ juju status vault --format yaml | grep charm-version
    charm-version: 220f0ee

Note: you should get the same output when deploying a local charm that includes a version file.

Bug reference

https://bugs.launchpad.net/juju/+bug/1812227

@achilleasa achilleasa changed the title Fix 1812227 Populate charm version for charms fetched from the charm store Feb 6, 2019
@achilleasa achilleasa changed the base branch from develop to 2.5 February 6, 2019 17:42
Copy link
Member

@SimonRichardson SimonRichardson left a comment

Choose a reason for hiding this comment

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

@achilleasa do we need a test in status to show that this works?

@achilleasa
Copy link
Contributor Author

@SimonRichardson I think the status code will eventually read from mongo so I didn't add a test in there as the bug was caused by the code not persisting the version to the store. Happy to add an extra test if you think it's important.

@achilleasa
Copy link
Contributor Author

!!build!!

@timClicks
Copy link

timClicks commented Feb 6, 2019

The current tests for juju show-status are quite brittle. They typically do exact string matching. So it's possible that this fix will actually break existing tests. because they are reliant on broken behaviour.

@achilleasa achilleasa force-pushed the fix-1812227 branch 4 times, most recently from e3e634e to 9b7f87a Compare February 7, 2019 16:59
@achilleasa
Copy link
Contributor Author

achilleasa commented Feb 7, 2019

@SimonRichardson I have pushed a new commit which hopefully should get all broken tests passing except these 2 that I am not really sure how to fix:

Any ideas for fixing these ones?

After reverting the series-related changes in charm.v6 with juju/charm#270 all tests should pass as expected so the above comment does not apply anymore.

@achilleasa
Copy link
Contributor Author

!!build!!

@SimonRichardson
Copy link
Member

@mitechie so when a charm is built it will inject a version file into the charm archive. The question is, if you create a charm without a build step the version file may or may not be included. If the version file is missing the status just hides the charm version, the charm creator can remedy this by putting a version file into the charm archive. Is this satisfactory?

Copy link
Member

@SimonRichardson SimonRichardson left a comment

Choose a reason for hiding this comment

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

LGTM

@achilleasa
Copy link
Contributor Author

$$merge$$

2 similar comments
@achilleasa
Copy link
Contributor Author

$$merge$$

@achilleasa
Copy link
Contributor Author

$$merge$$

@jujubot jujubot merged commit 9b812d5 into juju:2.5 Feb 11, 2019
@achilleasa achilleasa deleted the fix-1812227 branch February 11, 2019 17:02
@jameinel jameinel mentioned this pull request Feb 13, 2019
jujubot added a commit that referenced this pull request Feb 13, 2019
#9748

## Description of change

This just brings develop up-to-date with all the 2.5 patches:
 prdesc Merge pull request #9745 from jameinel/2.5-lease-invalid-retries-1815719
 prdesc Merge pull request #9740 from wallyworld/cmr-multi-offer-fix
 prdesc Merge pull request #9742 from jameinel/2.5-worker-lease-1815468
 prdesc Merge pull request #9736 from jameinel/2.5-leadership-client
 prdesc Merge pull request #9737 from jameinel/2.5-worker-lease-1815468
 prdesc Merge pull request #9722 from achilleasa/fix-1812227
 prdesc Merge pull request #9734 from babbageclunk/state-worker-dep-message
 prdesc Merge pull request #9733 from babbageclunk/raftlease-stop-global-clock
 prdesc Merge pull request #9735 from howbazaar/2.5-mongo-systemd-ulimit
 prdesc Merge pull request #9724 from babbageclunk/raftlease-upgrade-blank
 prdesc Merge pull request #9731 from howbazaar/2.5-status-close-error
 prdesc Merge pull request #9730 from howbazaar/2.5-lease-race
 prdesc Merge pull request #9727 from achilleasa/fix-1814638
 prdesc Merge pull request #9728 from wallyworld/rename-delete-storage-pool
 prdesc Merge pull request #9712 from jameinel/2.5-leases-nextTick
 prdesc Merge pull request #9709 from jameinel/2.5-update-testing-clock

## QA steps

See individual patches.

## Documentation changes

See individual patches.

## Bug reference

 prdesc https://bugs.launchpad.net/juju/+bug/1815719
 prdesc https://bugs.launchpad.net/juju/+bug/1813151
 prdesc https://bugs.launchpad.net/juju/+bug/1815179
 prdesc https://bugs.launchpad.net/juju/+bug/1815471
 prdesc https://bugs.launchpad.net/juju/+bug/1815468
 prdesc https://bugs.launchpad.net/juju/+bug/1812227
 prdesc https://bugs.launchpad.net/juju/+bug/1815405
 prdesc https://bugs.launchpad.net/juju/+bug/1813996
 prdesc https://bugs.launchpad.net/juju/+bug/1813995
 prdesc https://bugs.launchpad.net/juju/+bug/1815397
 prdesc https://bugs.launchpad.net/juju/+bug/1814638
 prdesc https://bugs.launchpad.net/juju/+bug/1814556
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants