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

Fix behaviour of relation-get hook tool when no --app flag passed #16938

Merged
merged 1 commit into from
Feb 16, 2024

Conversation

Aflynn50
Copy link
Contributor

The relation-get hook tool defaults to getting info about the unit relation. It has an --app flag to get information about the application relation instead.

The documentation for the function that determins which relation to get (determineUnitOrAppName) states that if no --app flag is provided and an app name is passed then an error should be thrown. This is not what happens.

I have added in an error that is returned in this case, and an extra check to see if it is a valid unit name or not (i.e. it could be an invalid unit name and application name). I have updated the unit tests to reflect this and removed a test confirming the erroneous behavior.

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

Unit tests

go test ./worker/uniter/runner/jujuc

Black box test

# Setup
juju bootstrap lxd lxd-dev
juju add-model default
juju deploy ./testcharms/charms/dummy-sink
juju deploy ./testcharms/charms/dummy-source
# Wait for machines
juju integrate dummy-sink dummy-source

# Test app name with no --app flag
juju exec --unit dummy-sink/0 "relation-get -r 0 - dummy-sink"   
# ERROR expected unit name, got applicaiton name dummy-sink

# Test invalid unit name
juju exec --unit dummy-sink/0 "relation-get -r 0 - dummy-sink//0"
# ERROR invalid unit name dummy-sink//0

Links

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

check for invalid unit name. This matches the function to it stated
behaviour in the comment.

Remove tests that confirmed wrong behavior and add tests of new
behaviour.
@Aflynn50 Aflynn50 requested a review from anvial February 16, 2024 13:29
Copy link
Member

@anvial anvial left a comment

Choose a reason for hiding this comment

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

I'm OK with code, and waiting simple.canonical.com to return back for QA.

worker/uniter/runner/jujuc/relation-get.go Show resolved Hide resolved
Copy link
Member

@anvial anvial left a comment

Choose a reason for hiding this comment

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

LG2M

@Aflynn50
Copy link
Contributor Author

/merge

@jujubot
Copy link
Collaborator

jujubot commented Feb 16, 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 jujubot merged commit 4299dd6 into juju:3.3 Feb 16, 2024
20 of 23 checks passed
@Aflynn50 Aflynn50 mentioned this pull request Feb 16, 2024
jujubot added a commit that referenced this pull request Feb 20, 2024
#16939

There were no merge conflicts from this, the commits added are:

#16844 Support minikube env folder on juju snap.
#16876 Pass all the availability zones when discovering subnets in clustered lxd.
#16890 Revert dot-minikube on snapcraft until auto-connect is approved.
#16567 Add lease manager worker documentation.
#16898 Suppress klog log messages. 
#16905 Ensure we close client.
#16915 Merge branch '3.1' of https://github.com/juju/juju into 3.1-into-3.3.
#16917 Fix docker cache for github actions.
#16922 Fail actions which are interrupted due to an agent restart in the middle.
#16930 Fix the HasModelAdmin API to handle non admins properly.
#16936 Merge branch '2.9' into merge-2.9-3.1. 
#16937 Merge branch '3.1' into merge-3.1-3.3.
#16938 Add check for relation-get with app arguemnt but no --app flag.
@Aflynn50 Aflynn50 mentioned this pull request Feb 20, 2024
jujubot added a commit that referenced this pull request Feb 20, 2024
#16951

The only merge conflicts were juju version numbers, all resolved to 3.5.



- #16939 
 - #16844 
 - #16876 
 - #16890 
 - #16567 
 - #16898 
 - #16905 
 - #16915 
 - #16917 
 - #16922 
 - #16930 
 - #16936 
 - #16937 
 - #16938 
- #16946
- #16934
- #16928
- #16889
- #16883
- #16880
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants