Documented and checked ActionEvent.set_results with "Stdout" key

[PietroPasotti]

Stderr output emitted during an action hook execution gets injected in an action's results by juju.

• documented this in the ActionEvent docstring
• added a check that users don't manually set_results to anything containing "Stdout", because it will either be overridden or override juju's or break something, not sure which one but either way, it ain't good.

Checklist

• [x] Do error messages, if any, present charm authors or operators with enough information to solve the problem?

QA steps

Added test for the new error condition.

Documentation changes

We should make this clear in the ActionEvent-related documentation. Already updated: https://discourse.charmhub.io/t/action-name-action-event/6466

Changelog

Please include a bulleted list of items here, suitable for inclusion in a release changelog.

• Added check that ActionEvent.set_results will fail if the results contain the 'special' juju key "Stdout"

[rwcarlsen]

Ha! - I actually fixed a bug in juju a while back related to this: https://github.com/juju/juju/pull/13748 - action-set should give you an error now (in new enough versions of juju). So we don't need our own framework error, but possibly a framework-level error can be "nicer".

[PietroPasotti]

Ha! - I actually fixed a bug in juju a while back related to this: juju/juju#13748 - action-set should give you an error now (in new enough versions of juju). So we don't need our own framework error, but possibly a framework-level error can be "nicer".

Right, I still think it's best to raise our own error instead to wait for juju to error out.

[PietroPasotti]

Here are the known "special" keys: "stdout", "stdout-encoding", "stderr", "stderr-encoding". Let's check for those as well. I found the juju source where the keys are being defined/used, but there is an issue: "Stderr" seems to be capitalized in this sample output. I don't see in the juju code where this capitalization is done, I'm wondering if it's an artifact from the juju client displaying the action results, rather than juju itself. It is suspicious that admin-password is not capitalized, however. Should I include the capitalized or the non-capitalized version of "stdout" in the 'reserved' keys? And what about the other keys? Is it Stdout-encoding or Stdout-Encoding, or...?

[juanmanuel-tirado]

In 2.9 there is a piece of code that changes these keys to start with uppercase when running into compat mode. Check here. This has been changed in 3.0 here

[PietroPasotti]

Thanks! From reading that code it seems that ReturnCode is also a reserved key.

[PietroPasotti]

I just noticed that the regex we use to validate the action result key '^[a-z0-9](([a-z0-9-.]+)?[a-z0-9])?$'actually precludes using uppercase letters altogether. This was added I think by @jnsgruk. Is the regex too strict, or were you aware of these issues and took a different solution, or something else..?

[jnsgruk]

I just noticed that the regex we use to validate the action result key '^[a-z0-9](([a-z0-9-.]+)?[a-z0-9])?$'actually precludes using uppercase letters altogether. This was added I think by @jnsgruk. Is the regex too strict, or were you aware of these issues and took a different solution, or something else..?

Interesting. Pretty sure a based the regex off something that lives in the Juju codebase (I can check a bit later if no one beats me to it!)

[rwcarlsen]

Do the juju checks need to be updated to check for ReturnCode (in addition to the stderr/out, etc.)?

[PietroPasotti]

Do you mean the code on the juju side?

[jnsgruk]

I just noticed that the regex we use to validate the action result key '^[a-z0-9](([a-z0-9-.]+)?[a-z0-9])?$'actually precludes using uppercase letters altogether. This was added I think by @jnsgruk. Is the regex too strict, or were you aware of these issues and took a different solution, or something else..?

Here it is: https://github.com/juju/juju/blob/137a772ed339b73b856e9adc0a5624976c2890b2/worker/uniter/runner/jujuc/action-set.go#L85

The action-set command validates that keys start with lowercase letters, so that's why the regex is as such in ops :)

[PietroPasotti]

Allright, so it seems that for now we're in the clear -- attempting to set Stdout/Stderr/ReturnCode and company will result into an error, not because they're 'protected keys', but because they contain uppercase characters. I'll reshape this PR to only include documentation changes.

[rwcarlsen]

When I last dealt with this trying to solve https://github.com/canonical/operator/issues/689 - somewhere in juju, the lower-case-key names were still being clobbered by the CamelCase special ones (e.g. results "stdout" was clobbered by juju's special "Stdout").

[rwcarlsen]

I think the conclusion was that checks in juju should cover the relevant clobbering concerns. So this PR is just doc clarifications and such.