Expose Command Structs for Plugins

[Mcrich23]

Type of Change

• Bug fix
• New feature
• Breaking change
• Documentation update

Motivation and Context

Plugins technically exist, but to add shortcuts or to do existing things with functions in container requires calling a compiled binary. This pull request aims to remove that hurdle and instability by exposing commands as a new ContainerCLI target.

Simply import ContainerCLI and you can access almost any command as if it were a native part of the binary. This makes plugin development significantly easier.

Testing

• Tested locally
• Added/updated tests
• Added/updated docs

No new tests or documentation in code needed?

[Mcrich23]

@jglogan This also accomplishes some of the work that I found necessary when developing my initial compose effort, because it increases safety and reduces the developer workload to reproduce, funnel, or output commands being run in the background. It also will encourage devs to use the commands internal to container instead of modifying containers and other things directly.

[jglogan]

How are you running the commands? Are you using ParseableCommand.parse()?

[Mcrich23]

You can do that, or just init it and then manually set every property after it.

Because of how Swift parser operates, we can't just init it with all of the properties, so we have to instead init it empty and then set all of the properties.

An example of this is in my Compose proposal.

[jglogan]

Having the ParseableCommand types public so we can all import the library and use parse() totally makes sense.

I'd prefer that that be just the API, instead of making it so that any change to the member properties is potentially a breaking change. Could we start with this PR simply allowing clients to use parse(), without changing member visibility?

[Mcrich23]

@jglogan We could put that in the documentation, but AsyncParsableCommand requires an empty public init to be specified in any public structure that conforms to it, so it would have to be available technically.

[Mcrich23]

However, we could make all properties and functions private to force use of the parse method.

[jglogan]

However, we could make all properties and functions private to force use of the parse method.

Exactly! Could you try that approach - making the CLI library such that you can import it and use parse, but the property visibility isn't public? I'm curious how that'd work for your plugin.

[jglogan]

Also, do you have an enhancement issue open for making the CLI library importable? If not, please create one and I'll assign it to you. It'd be best to have an issue backing each PR (especially for larger changes or those affecting the API). Thanks!

[Mcrich23]

I didn't have one, so I just made #609. But as I try to update my Compose plugin to use entirely parse, it is becoming increasingly difficult to pass down global flags that I want to respect inherently such as Flags.Global.verbose or Flags.Progress.disableProgressUpdates.

I strongly think that a great DX is the only way that people will make plugins for compose, so I think we kinda only have two ways to move forward. Either, we expose every property on commands, or we can expose flag groups in a setable manner that allows developers to pass down flag groups.

If we do the latter, this would not be required because .parse() would set them by default, but it would also allow them to be easily overriden and treated in a sort of "environment" manner.

[Mcrich23]

@jglogan We can undo it, but I decided to go ahead and make all of the OptionGroup properties public to make this possible. Again these are not required to be set when using .parse it just makes passing down use flags far easier.

[Mcrich23]

Actually, before we merge this, should I update the readme with information about developing plugins?

[jglogan]

Nah, for that, could you create a git issue for plugin development information? You and I can put together an outline there first, and then we'll do that in a dedicated PR.

It's long overdue. I have some thoughts scratched down in a local branch but have been getting sidetracked a lot.

[jglogan]

Builds now! I went through everything and left a few comments regarding some of the private methods that went to default visibility. Was that needed to compile the code without errors? For those (especially the table output stuff which we should just solve in a better way), I'm inclined to leave these private until someone actually needs to use these functions.

[jglogan]

Also: https://github.com/apple/container/blob/011e071495b85972fd2c0a32e4fdf29328361bde/Sources/CLI/System/SystemProperty.swift

And the files System/Property/*.swift should get the same treatment.

[Mcrich23]

Sounds good. I will fix these soon

[katiewasnothere]

@Mcrich23 Do you have an example of a plugin you're hoping to use these changes with? Draft code is fine, I'm just curious to see how it looks

[Mcrich23]

@katiewasnothere yeah. This hasn't been updated entirely for the finalized interface, but it and the conversation here should give you a decent idea of the current API with ContainerCommands. https://github.com/Mcrich23/container/blob/add-compose/Plugins/Compose/ComposeCLI/Commands/ComposeUp.swift

[dcantah]

I'm sufficiently dumb, but what is this target for?

[Mcrich23]

This is the new compose executable entry point since I could not otherwise get an executable and normal target for the same sources. So this is just a proxy making it executable.

[jglogan]

Looks like we need make fmt once more...

[Mcrich23]

Type of Change

• Bug fix
• New feature
• Breaking change
• Documentation update

Motivation and Context

Plugins technically exist, but to add shortcuts or to do existing things with functions in container requires calling a compiled binary. This pull request aims to remove that hurdle and instability by exposing commands as a new ContainerCLI target.

Simply import ContainerCLI and you can access almost any command as if it were a native part of the binary. This makes plugin development significantly easier.

Testing

• Tested locally
• Added/updated tests
• Added/updated docs

No new tests or documentation in code needed?

[Mcrich23]

@jglogan This also accomplishes some of the work that I found necessary when developing my initial compose effort, because it increases safety and reduces the developer workload to reproduce, funnel, or output commands being run in the background. It also will encourage devs to use the commands internal to container instead of modifying containers and other things directly.

[jglogan]

How are you running the commands? Are you using ParseableCommand.parse()?

[Mcrich23]

You can do that, or just init it and then manually set every property after it.

Because of how Swift parser operates, we can't just init it with all of the properties, so we have to instead init it empty and then set all of the properties.

An example of this is in my Compose proposal.

[jglogan]

Having the ParseableCommand types public so we can all import the library and use parse() totally makes sense.

I'd prefer that that be just the API, instead of making it so that any change to the member properties is potentially a breaking change. Could we start with this PR simply allowing clients to use parse(), without changing member visibility?

[Mcrich23]

@jglogan We could put that in the documentation, but AsyncParsableCommand requires an empty public init to be specified in any public structure that conforms to it, so it would have to be available technically.

[Mcrich23]

However, we could make all properties and functions private to force use of the parse method.

[jglogan]

However, we could make all properties and functions private to force use of the parse method.

Exactly! Could you try that approach - making the CLI library such that you can import it and use parse, but the property visibility isn't public? I'm curious how that'd work for your plugin.

[jglogan]

Also, do you have an enhancement issue open for making the CLI library importable? If not, please create one and I'll assign it to you. It'd be best to have an issue backing each PR (especially for larger changes or those affecting the API). Thanks!

[Mcrich23]

I didn't have one, so I just made #609. But as I try to update my Compose plugin to use entirely parse, it is becoming increasingly difficult to pass down global flags that I want to respect inherently such as Flags.Global.verbose or Flags.Progress.disableProgressUpdates.

I strongly think that a great DX is the only way that people will make plugins for compose, so I think we kinda only have two ways to move forward. Either, we expose every property on commands, or we can expose flag groups in a setable manner that allows developers to pass down flag groups.

If we do the latter, this would not be required because .parse() would set them by default, but it would also allow them to be easily overriden and treated in a sort of "environment" manner.

[Mcrich23]

@jglogan We can undo it, but I decided to go ahead and make all of the OptionGroup properties public to make this possible. Again these are not required to be set when using .parse it just makes passing down use flags far easier.

[Mcrich23]

Actually, before we merge this, should I update the readme with information about developing plugins?

[jglogan]

Nah, for that, could you create a git issue for plugin development information? You and I can put together an outline there first, and then we'll do that in a dedicated PR.

It's long overdue. I have some thoughts scratched down in a local branch but have been getting sidetracked a lot.

[jglogan]

Builds now! I went through everything and left a few comments regarding some of the private methods that went to default visibility. Was that needed to compile the code without errors? For those (especially the table output stuff which we should just solve in a better way), I'm inclined to leave these private until someone actually needs to use these functions.

[jglogan]

Also: https://github.com/apple/container/blob/011e071495b85972fd2c0a32e4fdf29328361bde/Sources/CLI/System/SystemProperty.swift

And the files System/Property/*.swift should get the same treatment.

[Mcrich23]

Sounds good. I will fix these soon

[katiewasnothere]

@Mcrich23 Do you have an example of a plugin you're hoping to use these changes with? Draft code is fine, I'm just curious to see how it looks

[Mcrich23]

@katiewasnothere yeah. This hasn't been updated entirely for the finalized interface, but it and the conversation here should give you a decent idea of the current API with ContainerCommands. https://github.com/Mcrich23/container/blob/add-compose/Plugins/Compose/ComposeCLI/Commands/ComposeUp.swift

[dcantah]

I'm sufficiently dumb, but what is this target for?

[Mcrich23]

This is the new compose executable entry point since I could not otherwise get an executable and normal target for the same sources. So this is just a proxy making it executable.

[jglogan]

Looks like we need make fmt once more...

[Mcrich23]

Type of Change

• Bug fix
• New feature
• Breaking change
• Documentation update

Motivation and Context

Plugins technically exist, but to add shortcuts or to do existing things with functions in container requires calling a compiled binary. This pull request aims to remove that hurdle and instability by exposing commands as a new ContainerCLI target.

Simply import ContainerCLI and you can access almost any command as if it were a native part of the binary. This makes plugin development significantly easier.

Testing

• Tested locally
• Added/updated tests
• Added/updated docs

No new tests or documentation in code needed?

[Mcrich23]

@jglogan This also accomplishes some of the work that I found necessary when developing my initial compose effort, because it increases safety and reduces the developer workload to reproduce, funnel, or output commands being run in the background. It also will encourage devs to use the commands internal to container instead of modifying containers and other things directly.

[jglogan]

How are you running the commands? Are you using ParseableCommand.parse()?

[Mcrich23]

You can do that, or just init it and then manually set every property after it.

Because of how Swift parser operates, we can't just init it with all of the properties, so we have to instead init it empty and then set all of the properties.

An example of this is in my Compose proposal.

[jglogan]

Having the ParseableCommand types public so we can all import the library and use parse() totally makes sense.

I'd prefer that that be just the API, instead of making it so that any change to the member properties is potentially a breaking change. Could we start with this PR simply allowing clients to use parse(), without changing member visibility?

[Mcrich23]

@jglogan We could put that in the documentation, but AsyncParsableCommand requires an empty public init to be specified in any public structure that conforms to it, so it would have to be available technically.

[Mcrich23]

However, we could make all properties and functions private to force use of the parse method.

[jglogan]

However, we could make all properties and functions private to force use of the parse method.

Exactly! Could you try that approach - making the CLI library such that you can import it and use parse, but the property visibility isn't public? I'm curious how that'd work for your plugin.

[jglogan]

Also, do you have an enhancement issue open for making the CLI library importable? If not, please create one and I'll assign it to you. It'd be best to have an issue backing each PR (especially for larger changes or those affecting the API). Thanks!

[Mcrich23]

I didn't have one, so I just made #609. But as I try to update my Compose plugin to use entirely parse, it is becoming increasingly difficult to pass down global flags that I want to respect inherently such as Flags.Global.verbose or Flags.Progress.disableProgressUpdates.

I strongly think that a great DX is the only way that people will make plugins for compose, so I think we kinda only have two ways to move forward. Either, we expose every property on commands, or we can expose flag groups in a setable manner that allows developers to pass down flag groups.

If we do the latter, this would not be required because .parse() would set them by default, but it would also allow them to be easily overriden and treated in a sort of "environment" manner.

[Mcrich23]

@jglogan We can undo it, but I decided to go ahead and make all of the OptionGroup properties public to make this possible. Again these are not required to be set when using .parse it just makes passing down use flags far easier.

[Mcrich23]

Actually, before we merge this, should I update the readme with information about developing plugins?

[jglogan]

Nah, for that, could you create a git issue for plugin development information? You and I can put together an outline there first, and then we'll do that in a dedicated PR.

It's long overdue. I have some thoughts scratched down in a local branch but have been getting sidetracked a lot.

[jglogan]

Builds now! I went through everything and left a few comments regarding some of the private methods that went to default visibility. Was that needed to compile the code without errors? For those (especially the table output stuff which we should just solve in a better way), I'm inclined to leave these private until someone actually needs to use these functions.

[jglogan]

Also: https://github.com/apple/container/blob/011e071495b85972fd2c0a32e4fdf29328361bde/Sources/CLI/System/SystemProperty.swift

And the files System/Property/*.swift should get the same treatment.

[Mcrich23]

Sounds good. I will fix these soon

[katiewasnothere]

@Mcrich23 Do you have an example of a plugin you're hoping to use these changes with? Draft code is fine, I'm just curious to see how it looks

[Mcrich23]

@katiewasnothere yeah. This hasn't been updated entirely for the finalized interface, but it and the conversation here should give you a decent idea of the current API with ContainerCommands. https://github.com/Mcrich23/container/blob/add-compose/Plugins/Compose/ComposeCLI/Commands/ComposeUp.swift

[dcantah]

I'm sufficiently dumb, but what is this target for?

[Mcrich23]

This is the new compose executable entry point since I could not otherwise get an executable and normal target for the same sources. So this is just a proxy making it executable.

[jglogan]

Looks like we need make fmt once more...

[Mcrich23]

Type of Change

• Bug fix
• New feature
• Breaking change
• Documentation update

Motivation and Context

Plugins technically exist, but to add shortcuts or to do existing things with functions in container requires calling a compiled binary. This pull request aims to remove that hurdle and instability by exposing commands as a new ContainerCLI target.

Simply import ContainerCLI and you can access almost any command as if it were a native part of the binary. This makes plugin development significantly easier.

Testing

• Tested locally
• Added/updated tests
• Added/updated docs

No new tests or documentation in code needed?

[Mcrich23]

@jglogan This also accomplishes some of the work that I found necessary when developing my initial compose effort, because it increases safety and reduces the developer workload to reproduce, funnel, or output commands being run in the background. It also will encourage devs to use the commands internal to container instead of modifying containers and other things directly.

[jglogan]

How are you running the commands? Are you using ParseableCommand.parse()?

[Mcrich23]

You can do that, or just init it and then manually set every property after it.

Because of how Swift parser operates, we can't just init it with all of the properties, so we have to instead init it empty and then set all of the properties.

An example of this is in my Compose proposal.

[jglogan]

Having the ParseableCommand types public so we can all import the library and use parse() totally makes sense.

I'd prefer that that be just the API, instead of making it so that any change to the member properties is potentially a breaking change. Could we start with this PR simply allowing clients to use parse(), without changing member visibility?

[Mcrich23]

@jglogan We could put that in the documentation, but AsyncParsableCommand requires an empty public init to be specified in any public structure that conforms to it, so it would have to be available technically.

[Mcrich23]

However, we could make all properties and functions private to force use of the parse method.

[jglogan]

However, we could make all properties and functions private to force use of the parse method.

Exactly! Could you try that approach - making the CLI library such that you can import it and use parse, but the property visibility isn't public? I'm curious how that'd work for your plugin.

[jglogan]

Also, do you have an enhancement issue open for making the CLI library importable? If not, please create one and I'll assign it to you. It'd be best to have an issue backing each PR (especially for larger changes or those affecting the API). Thanks!

[Mcrich23]

I didn't have one, so I just made #609. But as I try to update my Compose plugin to use entirely parse, it is becoming increasingly difficult to pass down global flags that I want to respect inherently such as Flags.Global.verbose or Flags.Progress.disableProgressUpdates.

I strongly think that a great DX is the only way that people will make plugins for compose, so I think we kinda only have two ways to move forward. Either, we expose every property on commands, or we can expose flag groups in a setable manner that allows developers to pass down flag groups.

If we do the latter, this would not be required because .parse() would set them by default, but it would also allow them to be easily overriden and treated in a sort of "environment" manner.

[Mcrich23]

@jglogan We can undo it, but I decided to go ahead and make all of the OptionGroup properties public to make this possible. Again these are not required to be set when using .parse it just makes passing down use flags far easier.

[Mcrich23]

Actually, before we merge this, should I update the readme with information about developing plugins?

[jglogan]

Nah, for that, could you create a git issue for plugin development information? You and I can put together an outline there first, and then we'll do that in a dedicated PR.

It's long overdue. I have some thoughts scratched down in a local branch but have been getting sidetracked a lot.

[jglogan]

Builds now! I went through everything and left a few comments regarding some of the private methods that went to default visibility. Was that needed to compile the code without errors? For those (especially the table output stuff which we should just solve in a better way), I'm inclined to leave these private until someone actually needs to use these functions.

[jglogan]

Also: https://github.com/apple/container/blob/011e071495b85972fd2c0a32e4fdf29328361bde/Sources/CLI/System/SystemProperty.swift

And the files System/Property/*.swift should get the same treatment.

[Mcrich23]

Sounds good. I will fix these soon

[katiewasnothere]

@Mcrich23 Do you have an example of a plugin you're hoping to use these changes with? Draft code is fine, I'm just curious to see how it looks

[Mcrich23]

@katiewasnothere yeah. This hasn't been updated entirely for the finalized interface, but it and the conversation here should give you a decent idea of the current API with ContainerCommands. https://github.com/Mcrich23/container/blob/add-compose/Plugins/Compose/ComposeCLI/Commands/ComposeUp.swift

[dcantah]

I'm sufficiently dumb, but what is this target for?

[Mcrich23]

This is the new compose executable entry point since I could not otherwise get an executable and normal target for the same sources. So this is just a proxy making it executable.

[jglogan]

Looks like we need make fmt once more...

[Mcrich23]

Type of Change

• Bug fix
• New feature
• Breaking change
• Documentation update

Motivation and Context

Plugins technically exist, but to add shortcuts or to do existing things with functions in container requires calling a compiled binary. This pull request aims to remove that hurdle and instability by exposing commands as a new ContainerCLI target.

Simply import ContainerCLI and you can access almost any command as if it were a native part of the binary. This makes plugin development significantly easier.

Testing

• Tested locally
• Added/updated tests
• Added/updated docs

No new tests or documentation in code needed?

[Mcrich23]

@jglogan This also accomplishes some of the work that I found necessary when developing my initial compose effort, because it increases safety and reduces the developer workload to reproduce, funnel, or output commands being run in the background. It also will encourage devs to use the commands internal to container instead of modifying containers and other things directly.

[jglogan]

How are you running the commands? Are you using ParseableCommand.parse()?

[Mcrich23]

You can do that, or just init it and then manually set every property after it.

Because of how Swift parser operates, we can't just init it with all of the properties, so we have to instead init it empty and then set all of the properties.

An example of this is in my Compose proposal.

[jglogan]

Having the ParseableCommand types public so we can all import the library and use parse() totally makes sense.

I'd prefer that that be just the API, instead of making it so that any change to the member properties is potentially a breaking change. Could we start with this PR simply allowing clients to use parse(), without changing member visibility?

[Mcrich23]

@jglogan We could put that in the documentation, but AsyncParsableCommand requires an empty public init to be specified in any public structure that conforms to it, so it would have to be available technically.

[Mcrich23]

However, we could make all properties and functions private to force use of the parse method.

[jglogan]

However, we could make all properties and functions private to force use of the parse method.

Exactly! Could you try that approach - making the CLI library such that you can import it and use parse, but the property visibility isn't public? I'm curious how that'd work for your plugin.

[jglogan]

Also, do you have an enhancement issue open for making the CLI library importable? If not, please create one and I'll assign it to you. It'd be best to have an issue backing each PR (especially for larger changes or those affecting the API). Thanks!

[Mcrich23]

I didn't have one, so I just made #609. But as I try to update my Compose plugin to use entirely parse, it is becoming increasingly difficult to pass down global flags that I want to respect inherently such as Flags.Global.verbose or Flags.Progress.disableProgressUpdates.

I strongly think that a great DX is the only way that people will make plugins for compose, so I think we kinda only have two ways to move forward. Either, we expose every property on commands, or we can expose flag groups in a setable manner that allows developers to pass down flag groups.

If we do the latter, this would not be required because .parse() would set them by default, but it would also allow them to be easily overriden and treated in a sort of "environment" manner.

[Mcrich23]

@jglogan We can undo it, but I decided to go ahead and make all of the OptionGroup properties public to make this possible. Again these are not required to be set when using .parse it just makes passing down use flags far easier.

[Mcrich23]

Actually, before we merge this, should I update the readme with information about developing plugins?

[jglogan]

Nah, for that, could you create a git issue for plugin development information? You and I can put together an outline there first, and then we'll do that in a dedicated PR.

It's long overdue. I have some thoughts scratched down in a local branch but have been getting sidetracked a lot.

[jglogan]

Builds now! I went through everything and left a few comments regarding some of the private methods that went to default visibility. Was that needed to compile the code without errors? For those (especially the table output stuff which we should just solve in a better way), I'm inclined to leave these private until someone actually needs to use these functions.

[jglogan]

Also: https://github.com/apple/container/blob/011e071495b85972fd2c0a32e4fdf29328361bde/Sources/CLI/System/SystemProperty.swift

And the files System/Property/*.swift should get the same treatment.

[Mcrich23]

Sounds good. I will fix these soon

[katiewasnothere]

@Mcrich23 Do you have an example of a plugin you're hoping to use these changes with? Draft code is fine, I'm just curious to see how it looks

[Mcrich23]

@katiewasnothere yeah. This hasn't been updated entirely for the finalized interface, but it and the conversation here should give you a decent idea of the current API with ContainerCommands. https://github.com/Mcrich23/container/blob/add-compose/Plugins/Compose/ComposeCLI/Commands/ComposeUp.swift

[dcantah]

I'm sufficiently dumb, but what is this target for?

[Mcrich23]

This is the new compose executable entry point since I could not otherwise get an executable and normal target for the same sources. So this is just a proxy making it executable.

[jglogan]

Looks like we need make fmt once more...

[Mcrich23]

Type of Change

• Bug fix
• New feature
• Breaking change
• Documentation update

Motivation and Context

Plugins technically exist, but to add shortcuts or to do existing things with functions in container requires calling a compiled binary. This pull request aims to remove that hurdle and instability by exposing commands as a new ContainerCLI target.

Simply import ContainerCLI and you can access almost any command as if it were a native part of the binary. This makes plugin development significantly easier.

Testing

• Tested locally
• Added/updated tests
• Added/updated docs

No new tests or documentation in code needed?

[Mcrich23]

@jglogan This also accomplishes some of the work that I found necessary when developing my initial compose effort, because it increases safety and reduces the developer workload to reproduce, funnel, or output commands being run in the background. It also will encourage devs to use the commands internal to container instead of modifying containers and other things directly.

[jglogan]

How are you running the commands? Are you using ParseableCommand.parse()?

[Mcrich23]

You can do that, or just init it and then manually set every property after it.

Because of how Swift parser operates, we can't just init it with all of the properties, so we have to instead init it empty and then set all of the properties.

An example of this is in my Compose proposal.

[jglogan]

Having the ParseableCommand types public so we can all import the library and use parse() totally makes sense.

I'd prefer that that be just the API, instead of making it so that any change to the member properties is potentially a breaking change. Could we start with this PR simply allowing clients to use parse(), without changing member visibility?

[Mcrich23]

@jglogan We could put that in the documentation, but AsyncParsableCommand requires an empty public init to be specified in any public structure that conforms to it, so it would have to be available technically.

[Mcrich23]

However, we could make all properties and functions private to force use of the parse method.

[jglogan]

However, we could make all properties and functions private to force use of the parse method.

Exactly! Could you try that approach - making the CLI library such that you can import it and use parse, but the property visibility isn't public? I'm curious how that'd work for your plugin.

[jglogan]

Also, do you have an enhancement issue open for making the CLI library importable? If not, please create one and I'll assign it to you. It'd be best to have an issue backing each PR (especially for larger changes or those affecting the API). Thanks!

[Mcrich23]

I didn't have one, so I just made #609. But as I try to update my Compose plugin to use entirely parse, it is becoming increasingly difficult to pass down global flags that I want to respect inherently such as Flags.Global.verbose or Flags.Progress.disableProgressUpdates.

I strongly think that a great DX is the only way that people will make plugins for compose, so I think we kinda only have two ways to move forward. Either, we expose every property on commands, or we can expose flag groups in a setable manner that allows developers to pass down flag groups.

If we do the latter, this would not be required because .parse() would set them by default, but it would also allow them to be easily overriden and treated in a sort of "environment" manner.

[Mcrich23]

@jglogan We can undo it, but I decided to go ahead and make all of the OptionGroup properties public to make this possible. Again these are not required to be set when using .parse it just makes passing down use flags far easier.

[Mcrich23]

Actually, before we merge this, should I update the readme with information about developing plugins?

[jglogan]

Nah, for that, could you create a git issue for plugin development information? You and I can put together an outline there first, and then we'll do that in a dedicated PR.

It's long overdue. I have some thoughts scratched down in a local branch but have been getting sidetracked a lot.

[jglogan]

Builds now! I went through everything and left a few comments regarding some of the private methods that went to default visibility. Was that needed to compile the code without errors? For those (especially the table output stuff which we should just solve in a better way), I'm inclined to leave these private until someone actually needs to use these functions.

[jglogan]

Also: https://github.com/apple/container/blob/011e071495b85972fd2c0a32e4fdf29328361bde/Sources/CLI/System/SystemProperty.swift

And the files System/Property/*.swift should get the same treatment.

[Mcrich23]

Sounds good. I will fix these soon

[katiewasnothere]

@Mcrich23 Do you have an example of a plugin you're hoping to use these changes with? Draft code is fine, I'm just curious to see how it looks

[Mcrich23]

@katiewasnothere yeah. This hasn't been updated entirely for the finalized interface, but it and the conversation here should give you a decent idea of the current API with ContainerCommands. https://github.com/Mcrich23/container/blob/add-compose/Plugins/Compose/ComposeCLI/Commands/ComposeUp.swift

[dcantah]

I'm sufficiently dumb, but what is this target for?

[Mcrich23]

This is the new compose executable entry point since I could not otherwise get an executable and normal target for the same sources. So this is just a proxy making it executable.

[jglogan]

Looks like we need make fmt once more...

[Mcrich23]

Type of Change

• Bug fix
• New feature
• Breaking change
• Documentation update

Motivation and Context

Plugins technically exist, but to add shortcuts or to do existing things with functions in container requires calling a compiled binary. This pull request aims to remove that hurdle and instability by exposing commands as a new ContainerCLI target.

Simply import ContainerCLI and you can access almost any command as if it were a native part of the binary. This makes plugin development significantly easier.

Testing

• Tested locally
• Added/updated tests
• Added/updated docs

No new tests or documentation in code needed?

[Mcrich23]

@jglogan This also accomplishes some of the work that I found necessary when developing my initial compose effort, because it increases safety and reduces the developer workload to reproduce, funnel, or output commands being run in the background. It also will encourage devs to use the commands internal to container instead of modifying containers and other things directly.

[jglogan]

How are you running the commands? Are you using ParseableCommand.parse()?

[Mcrich23]

You can do that, or just init it and then manually set every property after it.

Because of how Swift parser operates, we can't just init it with all of the properties, so we have to instead init it empty and then set all of the properties.

An example of this is in my Compose proposal.

[jglogan]

Having the ParseableCommand types public so we can all import the library and use parse() totally makes sense.

I'd prefer that that be just the API, instead of making it so that any change to the member properties is potentially a breaking change. Could we start with this PR simply allowing clients to use parse(), without changing member visibility?

[Mcrich23]

@jglogan We could put that in the documentation, but AsyncParsableCommand requires an empty public init to be specified in any public structure that conforms to it, so it would have to be available technically.

[Mcrich23]

However, we could make all properties and functions private to force use of the parse method.

[jglogan]

However, we could make all properties and functions private to force use of the parse method.

Exactly! Could you try that approach - making the CLI library such that you can import it and use parse, but the property visibility isn't public? I'm curious how that'd work for your plugin.

[jglogan]

Also, do you have an enhancement issue open for making the CLI library importable? If not, please create one and I'll assign it to you. It'd be best to have an issue backing each PR (especially for larger changes or those affecting the API). Thanks!

[Mcrich23]

I didn't have one, so I just made #609. But as I try to update my Compose plugin to use entirely parse, it is becoming increasingly difficult to pass down global flags that I want to respect inherently such as Flags.Global.verbose or Flags.Progress.disableProgressUpdates.

I strongly think that a great DX is the only way that people will make plugins for compose, so I think we kinda only have two ways to move forward. Either, we expose every property on commands, or we can expose flag groups in a setable manner that allows developers to pass down flag groups.

If we do the latter, this would not be required because .parse() would set them by default, but it would also allow them to be easily overriden and treated in a sort of "environment" manner.

[Mcrich23]

@jglogan We can undo it, but I decided to go ahead and make all of the OptionGroup properties public to make this possible. Again these are not required to be set when using .parse it just makes passing down use flags far easier.

[Mcrich23]

Actually, before we merge this, should I update the readme with information about developing plugins?

[jglogan]

Nah, for that, could you create a git issue for plugin development information? You and I can put together an outline there first, and then we'll do that in a dedicated PR.

It's long overdue. I have some thoughts scratched down in a local branch but have been getting sidetracked a lot.

[jglogan]

Builds now! I went through everything and left a few comments regarding some of the private methods that went to default visibility. Was that needed to compile the code without errors? For those (especially the table output stuff which we should just solve in a better way), I'm inclined to leave these private until someone actually needs to use these functions.

[jglogan]

Also: https://github.com/apple/container/blob/011e071495b85972fd2c0a32e4fdf29328361bde/Sources/CLI/System/SystemProperty.swift

And the files System/Property/*.swift should get the same treatment.

[Mcrich23]

Sounds good. I will fix these soon

[katiewasnothere]

@Mcrich23 Do you have an example of a plugin you're hoping to use these changes with? Draft code is fine, I'm just curious to see how it looks

[Mcrich23]

@katiewasnothere yeah. This hasn't been updated entirely for the finalized interface, but it and the conversation here should give you a decent idea of the current API with ContainerCommands. https://github.com/Mcrich23/container/blob/add-compose/Plugins/Compose/ComposeCLI/Commands/ComposeUp.swift

[dcantah]

I'm sufficiently dumb, but what is this target for?

[Mcrich23]

This is the new compose executable entry point since I could not otherwise get an executable and normal target for the same sources. So this is just a proxy making it executable.

[jglogan]

Looks like we need make fmt once more...

[Mcrich23]

Type of Change

• Bug fix
• New feature
• Breaking change
• Documentation update

Motivation and Context

Plugins technically exist, but to add shortcuts or to do existing things with functions in container requires calling a compiled binary. This pull request aims to remove that hurdle and instability by exposing commands as a new ContainerCLI target.

Simply import ContainerCLI and you can access almost any command as if it were a native part of the binary. This makes plugin development significantly easier.

Testing

• Tested locally
• Added/updated tests
• Added/updated docs

No new tests or documentation in code needed?

[Mcrich23]

@jglogan This also accomplishes some of the work that I found necessary when developing my initial compose effort, because it increases safety and reduces the developer workload to reproduce, funnel, or output commands being run in the background. It also will encourage devs to use the commands internal to container instead of modifying containers and other things directly.

[jglogan]

How are you running the commands? Are you using ParseableCommand.parse()?

[Mcrich23]

You can do that, or just init it and then manually set every property after it.

Because of how Swift parser operates, we can't just init it with all of the properties, so we have to instead init it empty and then set all of the properties.

An example of this is in my Compose proposal.

[jglogan]

Having the ParseableCommand types public so we can all import the library and use parse() totally makes sense.

I'd prefer that that be just the API, instead of making it so that any change to the member properties is potentially a breaking change. Could we start with this PR simply allowing clients to use parse(), without changing member visibility?

[Mcrich23]

@jglogan We could put that in the documentation, but AsyncParsableCommand requires an empty public init to be specified in any public structure that conforms to it, so it would have to be available technically.

[Mcrich23]

However, we could make all properties and functions private to force use of the parse method.

[jglogan]

However, we could make all properties and functions private to force use of the parse method.

Exactly! Could you try that approach - making the CLI library such that you can import it and use parse, but the property visibility isn't public? I'm curious how that'd work for your plugin.

[jglogan]

Also, do you have an enhancement issue open for making the CLI library importable? If not, please create one and I'll assign it to you. It'd be best to have an issue backing each PR (especially for larger changes or those affecting the API). Thanks!

[Mcrich23]

I didn't have one, so I just made #609. But as I try to update my Compose plugin to use entirely parse, it is becoming increasingly difficult to pass down global flags that I want to respect inherently such as Flags.Global.verbose or Flags.Progress.disableProgressUpdates.

I strongly think that a great DX is the only way that people will make plugins for compose, so I think we kinda only have two ways to move forward. Either, we expose every property on commands, or we can expose flag groups in a setable manner that allows developers to pass down flag groups.

If we do the latter, this would not be required because .parse() would set them by default, but it would also allow them to be easily overriden and treated in a sort of "environment" manner.

[Mcrich23]

@jglogan We can undo it, but I decided to go ahead and make all of the OptionGroup properties public to make this possible. Again these are not required to be set when using .parse it just makes passing down use flags far easier.

[Mcrich23]

Actually, before we merge this, should I update the readme with information about developing plugins?

[jglogan]

Nah, for that, could you create a git issue for plugin development information? You and I can put together an outline there first, and then we'll do that in a dedicated PR.

It's long overdue. I have some thoughts scratched down in a local branch but have been getting sidetracked a lot.

[jglogan]

Builds now! I went through everything and left a few comments regarding some of the private methods that went to default visibility. Was that needed to compile the code without errors? For those (especially the table output stuff which we should just solve in a better way), I'm inclined to leave these private until someone actually needs to use these functions.

[jglogan]

Also: https://github.com/apple/container/blob/011e071495b85972fd2c0a32e4fdf29328361bde/Sources/CLI/System/SystemProperty.swift

And the files System/Property/*.swift should get the same treatment.

[Mcrich23]

Sounds good. I will fix these soon

[katiewasnothere]

@Mcrich23 Do you have an example of a plugin you're hoping to use these changes with? Draft code is fine, I'm just curious to see how it looks

[Mcrich23]

@katiewasnothere yeah. This hasn't been updated entirely for the finalized interface, but it and the conversation here should give you a decent idea of the current API with ContainerCommands. https://github.com/Mcrich23/container/blob/add-compose/Plugins/Compose/ComposeCLI/Commands/ComposeUp.swift

[dcantah]

I'm sufficiently dumb, but what is this target for?

[Mcrich23]

This is the new compose executable entry point since I could not otherwise get an executable and normal target for the same sources. So this is just a proxy making it executable.

[jglogan]

Looks like we need make fmt once more...