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: add charm-rev to ApplicationStatus #17919

Open
wants to merge 1 commit into
base: 3.5
Choose a base branch
from
Open

fix: add charm-rev to ApplicationStatus #17919

wants to merge 1 commit into from
+1 −0

Conversation

amandahla
Copy link
Contributor

@amandahla amandahla commented Aug 13, 2024 •
edited
Loading

With this change will be possible get the revision from an Application.

Note: I'm targeting 3.1 since per CONTRIBUTING it should be targeted at the lowest version affected. Please let me know if a different one should be targeted instead.

Checklist

  • Code style: imports ordered, good names, simple structure, etc
  • Comments saying why design decisions were made
  • Go unit tests, with comments saying what you're testing
  • Integration tests, with comments saying what you're testing
  • doc.go added or updated in changed packages

QA steps

While creating a third-party client, is possible to get charm revision like this:

func (r *registry) parseStatus(status *params.FullStatus) {
	for applicationName, application := range status.Applications {
		r.jujuApplications.With(prometheus.Labels{
			"name":   applicationName,
			"status": application.Status.Status,
			"channel": application.CharmChannel,
			"revision": application.CharmRev,
		}).Set(checkStatus(application.Status.Status, []string{"active"}))
...

Documentation changes

No changes.

Links

To be used in https://github.com/neoaggelos/juju_exporter

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

Jira card: JUJU-

@jujubot
Copy link
Collaborator

jujubot commented Aug 13, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

9 similar comments
@jujubot
Copy link
Collaborator

jujubot commented Aug 13, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

@jujubot
Copy link
Collaborator

jujubot commented Aug 13, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

@jujubot
Copy link
Collaborator

jujubot commented Aug 13, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

@jujubot
Copy link
Collaborator

jujubot commented Aug 13, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

@jujubot
Copy link
Collaborator

jujubot commented Aug 13, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

@jujubot
Copy link
Collaborator

jujubot commented Aug 13, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

@jujubot
Copy link
Collaborator

jujubot commented Aug 13, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

@jujubot
Copy link
Collaborator

jujubot commented Aug 13, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

@jujubot
Copy link
Collaborator

jujubot commented Aug 13, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

@amandahla amandahla mentioned this pull request Aug 13, 2024
Open
2 tasks
@hpidcock hpidcock added the 3.1 label Aug 14, 2024
@SimonRichardson
Copy link
Member

@amandahla Thanks for opening the PR, we're not expecting to do another 3.1 release (other than security fixes), so it's best to target 3.5 or above.

In terms of the code, can you not use charm-version? That version is the one supplied by the charm itself.

@amandahla amandahla changed the base branch from 3.1 to 3.5 August 15, 2024 13:13
@amandahla
Copy link
Contributor Author

@amandahla Thanks for opening the PR, we're not expecting to do another 3.1 release (other than security fixes), so it's best to target 3.5 or above.

In terms of the code, can you not use charm-version? That version is the one supplied by the charm itself.

Hi, thanks, I changed it now.

I tried using the version but it was empty. I tested with k8s and machine charms.

@hpidcock hpidcock added 3.5 and removed 3.1 labels Aug 19, 2024
@hpidcock
Copy link
Member

/build

@hpidcock hpidcock assigned Aflynn50 and unassigned SimonRichardson Sep 26, 2024
SimonRichardson
SimonRichardson reviewed Sep 26, 2024
View reviewed changes
rpc/params/status.go
@@ -147,6 +147,7 @@ type ApplicationStatus struct {
CharmVersion string `json:"charm-version"`
CharmProfile string `json:"charm-profile"`
CharmChannel string `json:"charm-channel,omitempty"`
CharmRev int `json:"charm-rev,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

We'll need to wire CharmRev up for this to actually work.

Copy link
Member

Choose a reason for hiding this comment

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

Apply the following diff, I'd expect to see some test fallout once we do this...

diff --git a/apiserver/facades/client/client/status.go b/apiserver/facades/client/client/status.go
index 0c51848213..331b97b480 100644
--- a/apiserver/facades/client/client/status.go
+++ b/apiserver/facades/client/client/status.go
@@ -1392,6 +1392,7 @@ func (context *statusContext) processApplication(application *state.Application)
                Charm:        applicationCharm.URL(),
                CharmVersion: applicationCharm.Version(),
                CharmProfile: charmProfileName,
+               CharmRev:     applicationCharm.Revision(),
                CharmChannel: channel,
                Base: params.Base{
                        Name:    base.OS,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SimonRichardson SimonRichardson SimonRichardson left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@Aflynn50 Aflynn50

Labels
3.5
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@amandahla @jujubot @SimonRichardson @hpidcock @Aflynn50