[grafana] conf and logs from snap added #3289

GairePravesh · 2023-06-27T09:05:44Z

files added:
/var/snap/grafana/common/data/log/grafana.log*
/var/snap/grafana/current/conf/grafana.ini

cmds added:
snap info grafana
grafana.grafana-cli plugins ls
grafana.grafana-cli plugins list-remote

Closes: #3278

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 · 2023-06-27T09:14:03Z

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-3289
And now you can install the packages.

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

pmoravec · 2023-06-27T12:44:48Z

sos/report/plugins/grafana.py

+        if self._is_snap:
+            regexp = r"((?m)^\s*(%s)\s*=\s*)(.*)" % "|".join(protect_keys)
+            self.do_path_regex_sub(
+                "/var/snap/grafana/current/conf/grafana.ini",
+                regexp,
+                r"\1*********"
+            )
+        else:
+            regexp = r"((?m)^\s*(%s)\s*=\s*)(.*)" % "|".join(protect_keys)
+            self.do_path_regex_sub(
+                "/etc/grafana/grafana.ini",
+                regexp,
+                r"\1*********"
+            )


If the only change is in filename, maybe it is easier to have:

inifile = `"/var/snap/grafana/current/conf/grafana.ini"` if self_is_snap else "/etc/grafana/grafana.ini" regexp = r"((?m)^\s*(%s)\s*=\s*)(.*)" % "|".join(protect_keys) self.do_path_regex_sub( inifile, regexp, r"\1*********" )

pmoravec · 2023-06-27T12:45:10Z

sos/report/plugins/grafana.py

-            "admin_password", "secret_key"
+            "admin_password",
+            "secret_key",
+            "password",
+            "client_secret"


+1, that helps also to non-snap instances.

arif-ali

@GairePravesh Thanks for your contribution

probably nits more than anything

What about creating a variable called grafana_cli, and based if it is snap or standard, and then use that variable to do the grafana-cli commands. This way, if more commands are needed we add 1 line rather than duplication
We could probably do the same with both the log path and config path, as they would be similar

i.e.

if self._is_snap:
    grafana_cli = "grafana.grafana-cli"
else:
    grafana_cli = "grafana-cli"

self.add_cmd_output([
    "%s plugin ls" % grafana_cli,
    "%s plugin list remote" % grafana_cli,
    "%s -v" % grafana_cli,
])

pmoravec · 2023-06-27T12:54:57Z

Generally ACK, just a though: since the "snap instance or not" call flows differ a lot, it would maybe make more sense to have SnapPlugin class likewise we have RedHatPlugin or DebianPlugin? Though the Plugin hierarchy is likely distribution-based, not package-manager-based. But maybe if some other plugins might benefit from this in future, it should be easy to add SnapPlugin (likewise https://github.com/sosreport/sos/blob/main/sos/policies/distros/ubuntu.py#L49)?

arif-ali · 2023-06-27T15:03:14Z

Generally ACK, just a though: since the "snap instance or not" call flows differ a lot, it would maybe make more sense to have SnapPlugin class likewise we have RedHatPlugin or DebianPlugin? Though the Plugin hierarchy is likely distribution-based, not package-manager-based. But maybe if some other plugins might benefit from this in future, it should be easy to add SnapPlugin (likewise https://github.com/sosreport/sos/blob/main/sos/policies/distros/ubuntu.py#L49)?

I do like the idea, I will have a look at this in the background, and see how that looks with refactoring a few of the other plugins that has this already, like maas and lxd

TurboTurtle

if self._is_snap:
    grafana_cli = "grafana.grafana-cli"
else:
    grafana_cli = "grafana-cli"

self.add_cmd_output([
    "%s plugin ls" % grafana_cli,
    "%s plugin list remote" % grafana_cli,
    "%s -v" % grafana_cli,
])

This, but please note that f-strings are preferred per the contributions guidelines.

The intent of the changes looks good, just simplifying the flow as suggested above would be my 2 cents.

As far as a SnapPlugin goes, I'm not sure how well we'd be able to have that in tandem with distro-based enablement without things getting real fragile.

Perhaps instead we could add a helper method like self.is_pkg_snap('$pkg') (and accompanying is_pkg_rpm(), is_pkg_deb() etc...) to better support these checks within the current scope of enablement desgns?

files added: /var/snap/grafana/common/data/log/grafana.log* /var/snap/grafana/current/conf/grafana.ini cmds added: snap info grafana grafana.grafana-cli plugins ls grafana.grafana-cli plugins list-remote Closes: sosreport#3278 Signed-off-by: Pravesh Gaire <[email protected]>

arif-ali · 2023-08-16T07:55:20Z

sos/report/plugins/grafana.py

-        regexp = r"(^\s*(%s)\s*=\s*)(.*)" % "|".join(protect_keys)
-        self.do_path_regex_sub("/etc/grafana/grafana.ini",
-                               regexp, r"\1*********")
+        regexp = r"((?m)^\s*(%s)\s*=\s*)(.*)" % "|".join(protect_keys)


can you remove the (?m) as that is now implied by default in the self.do_path_regex_sub() based on issue #3301 and not going to be allowed in this manner in py311, thanks

TurboTurtle · 2023-08-16T13:59:49Z

sos/report/plugins/grafana.py

+            f'{grafana_cli} -v' if not self._is_snap else None,
+            'grafana-server -v' if not self._is_snap else None


Do these commands not work with a snap installation? Or are they not present. Non-present commands are handled gracefully by Plugin, so we don't need to set these to None and filter later.

Likewise we don't need to gate the snap info command, we can just tuck an add_cmd_output() under the conditional on line 30.

TurboTurtle · 2023-08-16T14:00:34Z

sos/report/plugins/grafana.py

-            "/etc/sysconfig/grafana-server",
+            log_path + log_file_pattern,
+            config_path,
+            "/etc/sysconfig/grafana-server" if not self._is_snap else None


add_copy_spec() also handles non-existing paths gracefully, so we can just have it listed here and it will be ignored for snap installations.

arif-ali · 2023-11-09T10:07:03Z

This is being closed in favour of #3410, Pravesh will not longer be contributing

pmoravec reviewed Jun 27, 2023

View reviewed changes

arif-ali reviewed Jun 27, 2023

View reviewed changes

TurboTurtle reviewed Jun 27, 2023

View reviewed changes

GairePravesh force-pushed the main branch 3 times, most recently from eca3de6 to ceb702f Compare August 16, 2023 07:20

GairePravesh force-pushed the main branch from ceb702f to 3314558 Compare August 16, 2023 07:44

Merge branch 'main' into main

589a760

arif-ali requested changes Aug 16, 2023

View reviewed changes

TurboTurtle reviewed Aug 16, 2023

View reviewed changes

arif-ali closed this Nov 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[grafana] conf and logs from snap added #3289

[grafana] conf and logs from snap added #3289

GairePravesh commented Jun 27, 2023 •

edited

Loading

packit-as-a-service bot commented Jun 27, 2023

pmoravec Jun 27, 2023

pmoravec Jun 27, 2023

arif-ali left a comment

pmoravec commented Jun 27, 2023

arif-ali commented Jun 27, 2023

TurboTurtle left a comment

arif-ali Aug 16, 2023

TurboTurtle Aug 16, 2023

TurboTurtle Aug 16, 2023

TurboTurtle Aug 16, 2023

arif-ali commented Nov 9, 2023

		f'{grafana_cli} -v' if not self._is_snap else None,
		'grafana-server -v' if not self._is_snap else None

[grafana] conf and logs from snap added #3289

[grafana] conf and logs from snap added #3289

Conversation

GairePravesh commented Jun 27, 2023 • edited Loading

packit-as-a-service bot commented Jun 27, 2023

pmoravec Jun 27, 2023

Choose a reason for hiding this comment

pmoravec Jun 27, 2023

Choose a reason for hiding this comment

arif-ali left a comment

Choose a reason for hiding this comment

pmoravec commented Jun 27, 2023

arif-ali commented Jun 27, 2023

TurboTurtle left a comment

Choose a reason for hiding this comment

arif-ali Aug 16, 2023

Choose a reason for hiding this comment

TurboTurtle Aug 16, 2023

Choose a reason for hiding this comment

TurboTurtle Aug 16, 2023

Choose a reason for hiding this comment

TurboTurtle Aug 16, 2023

Choose a reason for hiding this comment

arif-ali commented Nov 9, 2023

GairePravesh commented Jun 27, 2023 •

edited

Loading