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

Add support for exec "service-context" #957

Merged
merged 11 commits into from
Jul 17, 2023
Merged

Conversation

benhoyt
Copy link
Collaborator

@benhoyt benhoyt commented Jun 26, 2023

This is Pebble PR canonical/pebble#246

Also add working_dir field to pebble.Service and flesh out types for HttpDict, TcpDict, and ExecDict.

This is the ops part of OP034.

We should also document this at https://juju.is/docs/sdk/interact-with-pebble once this is merged.

@benhoyt benhoyt changed the title Add support for exec "context" Add support for exec "service-context" Jul 11, 2023
Copy link
Member

@jameinel jameinel left a comment

Choose a reason for hiding this comment

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

Is there anything where we should be checking the Pebble version and giving users nice messages if this feature is/isn't supported?

'group-id': Optional[int],
'group': str,
'working-dir': str},
total=False)
Copy link
Member

Choose a reason for hiding this comment

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

How did these get so out of sync with what you can put into them? Presumably because they are just dicts (so you could have always passed additional information there).
But that also implies that we are missing test cases that the extra fields have any impact on the requests.
I suppose mostly these are just curried to Pebble, and all the testing should happen at that level.
It does feel like we are missing something about the contract with an external system
(for example, someone might spell it service_context vs service-context, and while the typeddict will catch with a linter, it would be nice to have some form of validation that it matches what pebble actually expects)

What if pebble had a "validate these arguments" mode, where it didn't actually run it, but at least validated the content, and then an ops test that if it doesn't see pebble in the path, just gets skipped, but otherwise asks to validate args that we want to pass?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These types only just became public (in ops 2.4.0), so that's probably the main reason. I wanted them to be public so people could use them to type check their pebble.LayerDicts in their charms. If people are using this new type annotations for their dicts, it will check them (though they don't have to).

Separately, we're planning to add a --dry mode for validating layer config to Pebble (canonical/pebble#214). But I think additional work here is out of scope, at least for now.

@benhoyt
Copy link
Collaborator Author

benhoyt commented Jul 12, 2023

@jameinel I've updated the PR to add JujuVersion.supports_exec_service_context and used that in Container.exec to give a nice error if they're on a non-supporting version of Juju. I think I've got the Juju versions right, assuming juju/juju#15909 goes in and is merged forward.

return self.patch >= 6 # 3.1.6+ supports it
if self.minor == 2:
return self.patch >= 2 # 3.2.2+ supports it
return True # 3.3+ will
Copy link
Member

Choose a reason for hiding this comment

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

fwiw, 3.2.1 isn't "out" it just is a burned release number so nobody will ever run it.

Anyway, are we sure that this is a true statement? Have we already landed the pebble changes into the juju branches such that the next releases will have the service context?

Would this read cleaner as:

if (self.major, self.minor, self.patch) < (3,1,6):
  # First released in 3.1.6
  return False
if (self.major.self.minor,self.patch) == (3,2,0):
  # 3.2.0 was released before pebble was updated, but all other 3.2 releases have the change
  return False
return True

@benhoyt benhoyt merged commit 12b2da6 into canonical:main Jul 17, 2023
@benhoyt benhoyt deleted the exec-context branch July 17, 2023 02:58
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.

3 participants