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

Update to message recommendations asking to run pro #2936

Merged
merged 1 commit into from
Feb 12, 2024
Merged

Update to message recommendations asking to run pro #2936

merged 1 commit into from
Feb 12, 2024

Conversation

dheyay
Copy link
Contributor

@dheyay dheyay commented Jan 31, 2024

Why is this needed?

This PR solves all of our problems because...
Ensure 'sudo' is included for commands running pro which require root access.
Fixes: #2598

Test Steps

Checklist

  • I have updated or added any unit tests accordingly
  • I have updated or added any integration tests accordingly
  • Changes here need to be documented, and this was done in:

Does this PR require extra reviews?

  • Yes
  • No

@github-actions GitHub Actions
Copy link

Jira: This PR is not related to a Jira item. (The PR title does not include a SC-#### reference)

GitHub Issues:

Launchpad Bugs: No Launchpad bugs are fixed by this PR. (No commits have LP: #### references)

Documentation: The changes in this PR do not require documentation changes.

👍 this comment to confirm that this is correct.

@github-actions GitHub Actions
Copy link

🌎 This PR changes translatable messages. 🌏

Please select which scenarios apply. For further explanation, please read our policy on message changes.

  • New messages are being added.
    • We will ask translators to take a look and add translations if they have time, but it will not block this PR.
  • Existing messages are being changed.
    • ⚠️ Please add a comment with justification of why messages are being altered.
    • If the changes are trivial (e.g. a typo fix), then translations must be preserved.
    • If the changes are substantial, then we will ask translators to take a look and update translations if they have time, but it will not block this PR.
  • Existing messages are being deleted.
    • No special action needed.

@lucasmoura
Copy link
Contributor

There are two things missing on this PR:

  1. We need to update the existing integration tests that check that message:
    https://github.com/canonical/ubuntu-pro-client/blob/main/features/livepatch.feature#L54

  2. We need to update the translation files. To do that, just run the update-pos.sh script inside the tools folder and commit the result. But double check that it has not touched any other message besides the one that was modified.

@lucasmoura
Copy link
Contributor

@dheyay That looks great. I think we just need to handle the conflicts and we can land this PR

@dheyay dheyay force-pushed the pro-sudo-rec branch 2 times, most recently from fdc975a to 1d5c2c8 Compare February 6, 2024 17:06
orndorffgrant
orndorffgrant reviewed Feb 8, 2024
View reviewed changes
Copy link
Collaborator

@orndorffgrant orndorffgrant left a comment

Choose a reason for hiding this comment

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

Even though it is a logging statement, I think the pro config set suggestion in config.py around line 588 should have a sudo as well.

Otherwise this looks good!

@dheyay
Copy link
Contributor Author

dheyay commented Feb 8, 2024

Should be good to merge now

lucasmoura
lucasmoura reviewed Feb 8, 2024
View reviewed changes
uaclient/config.py
@@ -585,7 +585,7 @@ def warn_about_invalid_keys(self):
LOG.warning('legacy "ua_config" found in uaclient.conf')
LOG.warning("Please do the following:")
LOG.warning(
" 1. run `pro config set field=value` for each"
" 1. run `sudo pro config set field=value` for each"
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems we also have that same suggestion on postinst. I think we should update it there as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be good now @lucasmoura, thanks!

orndorffgrant
orndorffgrant approved these changes Feb 12, 2024
View reviewed changes
Copy link
Collaborator

@orndorffgrant orndorffgrant left a comment

Choose a reason for hiding this comment

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

Thank you!

@orndorffgrant orndorffgrant merged commit 111dd27 into next-v32 Feb 12, 2024
25 checks passed
@orndorffgrant orndorffgrant deleted the pro-sudo-rec branch February 12, 2024 21:27
@orndorffgrant orndorffgrant mentioned this pull request Feb 16, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lucasmoura lucasmoura lucasmoura left review comments

@orndorffgrant orndorffgrant orndorffgrant approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dheyay @lucasmoura @orndorffgrant