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

[juju][collect] Fix new juju collection #3400

Merged
merged 1 commit into from
Nov 16, 2023

Conversation

arif-ali
Copy link
Member

New version of juju uses colorisation, and therefore juju status and json.loads doesn't load the juju status correctly. By using --no-color based on the version of juju this should this particular use-case

Resolves: #3399
Resolves: SET-339


Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines

  • Is the commit message split over multiple lines and hard-wrapped at 72 characters?
  • Is the subject and message clear and concise?
  • Does the subject start with [plugin_name] if submitting a plugin patch or a [section_name] if part of the core sosreport code?
  • Does the commit contain a Signed-off-by: First Lastname [email protected]?
  • Are any related Issues or existing PRs properly referenced via a Closes (Issue) or Resolved (PR) line?

@packit-as-a-service
Copy link

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

  • sudo yum install -y dnf-plugins-core on RHEL 8
  • sudo dnf install -y dnf-plugins-core on Fedora
  • dnf copr enable packit/sosreport-sos-3400
  • And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

1 similar comment
@packit-as-a-service
Copy link

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

  • sudo yum install -y dnf-plugins-core on RHEL 8
  • sudo dnf install -y dnf-plugins-core on Fedora
  • dnf copr enable packit/sosreport-sos-3400
  • And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

@arif-ali arif-ali force-pushed the sos-arif-juju-collect-3-fix branch 2 times, most recently from 9e7bc4a to 90bfcff Compare October 31, 2023 13:33
@TurboTurtle
Copy link
Member

/packit rebuild-failed

Copy link
Member

@TurboTurtle TurboTurtle left a comment

Choose a reason for hiding this comment

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

LGTM.

We're trying to push f-strings, but here it'd be a bit awkward without re-writing the entire command slicing, so we'll put this one on the back burner for another time.

As an aside, I can't imagine we're the only ones calling juju so it may be worth working with the juju devs to automatically detect when to use --no-color from the juju command for situations like this (e.g. when not called from a shell).

Comment on lines 163 to 164
cmd = "juju version"
res = sos_get_command_output(cmd)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not single

res = sos_get_command_output("juju version")

?

(or maybe even merge it with return, though that might sacrifice code readability too much)

@arif-ali
Copy link
Member Author

As an aside, I can't imagine we're the only ones calling juju so it may be worth working with the juju devs to automatically detect when to use --no-color from the juju command for situations like this (e.g. when not called from a shell).

Actually, I didn't think too much of it, and I think that is valid point.

So, I have opened a bug on our end.

I think, at this point, I'm on the opinion that I would put this as a draft/wip, until we are 100%, and if it does get fixed in juju, then there's no point on this PR.

@arif-ali arif-ali marked this pull request as draft October 31, 2023 16:19
@arif-ali
Copy link
Member Author

arif-ali commented Nov 1, 2023

So, after discussing this, and update on the bug, https://bugs.launchpad.net/juju/+bug/2042309, the reason why we are getting colorise is due to the fact we are allocating PTY. Is there a way we can avoid this, and then we don't need to force the --no-color

         res = self.primary.run_command(cmd, get_pty=pty, need_root=need_root,
                                       timeout=timeout)

@TurboTurtle
Copy link
Member

IIRC, that is due to the default usage of ssh and some behavior of that we found during early testing. I'm open to making that toggle-able at the transport in use, which would allow juju to avoid it (and at the very least provide us the opportunity to re-test ssh).

On that note, it's weird that we're setting this in a Cluster profile, and not having it fully determined in the transport.

I think the best approach here is to remove the pty determination from cluster, let the transports determine it via a (to be added) property, with current behavior as the default and an override for juju.

I'm happy to move bits around on this (though likely in a few days as day job has me tied up for the week), but if you'd like to handle it within this or another PR, that's fine too, just let me know.

TurboTurtle added a commit to TurboTurtle/sos that referenced this pull request Nov 3, 2023
The `get_pty` parameter for remote executed commands was both a bit of a
misnomer and applied too broadly.

Refactor this to `use_shell` to be more obvious about what the intent
behind the option is, and default all transports to `False`, so that by
default we do not wrap any commands in a bash shell.

This may be overriden on a per-transport basis via the ned `_need_shell`
property within transport subclasses. Further, this facility has been
expanded to be allowed on a per-command basis from
`SoSNode.run_command()` and wherever that is linked.

Related: sosreport#3399
Related: sosreport#3400

Signed-off-by: Jake Hunsaker <[email protected]>
TurboTurtle added a commit that referenced this pull request Nov 7, 2023
The `get_pty` parameter for remote executed commands was both a bit of a
misnomer and applied too broadly.

Refactor this to `use_shell` to be more obvious about what the intent
behind the option is, and default all transports to `False`, so that by
default we do not wrap any commands in a bash shell.

This may be overriden on a per-transport basis via the ned `_need_shell`
property within transport subclasses. Further, this facility has been
expanded to be allowed on a per-command basis from
`SoSNode.run_command()` and wherever that is linked.

Related: #3399
Related: #3400

Signed-off-by: Jake Hunsaker <[email protected]>
@arif-ali
Copy link
Member Author

arif-ali commented Nov 7, 2023

/packit rebuild-failed

@arif-ali arif-ali marked this pull request as ready for review November 7, 2023 17:25
New version of juju uses colorisation, and therefore juju status
and json.loads doesn't load the juju status correctly. By using
--no-color based on the version of juju this should fix this
particular use-case

Resolves: sosreport#3399
Resolves: SET-339

Signed-off-by: Arif Ali <[email protected]>
@TurboTurtle TurboTurtle merged commit cf3af27 into sosreport:main Nov 16, 2023
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sos collect in Juju environment
3 participants