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

Implement a PayloadRegister as a factory to register and create Payloads #68

Merged
merged 35 commits into from
Mar 27, 2023

Conversation

jgsogo
Copy link
Contributor

@jgsogo jgsogo commented Mar 3, 2023

This PR implements a PayloadRegistry factory as a template specialization of plugin::registry::PluginRegistry<T> (see #63). This registry can instantiate PayloadInfo items which, in turn, are responsible of instantiate concrete Payload implementations (same schema as TargetSystem).

It also turns the ZipPayload into a pluggable Payload for this new registry.

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@jgsogo jgsogo changed the title [WIP] Implement a PayloadRegister as a factory to register and create Payloads Implement a PayloadRegister as a factory to register and create Payloads Mar 20, 2023
qssc::payload::registry::PayloadRegistry::registeredPlugins()) {
os << target.second.getName() << " - " << target.second.getDescription()
<< "\n";
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm listing here the available payloads, although there is no CLI argument to select one or another (currently there is only one: ZIP). If we agree on the argument name, I can add it to this PR, or we can wait until we have more payloads registered and there is an actual use case for this argument.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest adding the argument to this PR, for completeness. We have a similar arg for listing targets:

[$] ./bin/qss-compiler --show-targets
Registered Targets:
  --mock                                               - Mock system for testing the targetting infrastructure.

show-payloads seems appropriate. (Feel free also to correct the spelling of 'targeting'. :)

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

1 similar comment
@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@jgsogo jgsogo requested a review from vrpascuzzi March 22, 2023 08:23
Copy link
Contributor

@vrpascuzzi vrpascuzzi left a comment

Choose a reason for hiding this comment

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

Approving from my side.

Pinging @mhillenbrand one more, and @mbhealy time to take a look.

Copy link
Contributor

@mhillenbrand mhillenbrand left a comment

Choose a reason for hiding this comment

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

This PR is a great improvement! I find it already in very good shape - a few comments from a first look below.

lib/Payload/ZipPayload/Payload.inc Outdated Show resolved Hide resolved
test/unittest/Payload/PayloadRegistryTest.cpp Show resolved Hide resolved
lib/Payload/ZipPayload/ZipPayload.cpp Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@jgsogo jgsogo requested a review from mbhealy March 27, 2023 09:05
@jgsogo
Copy link
Contributor Author

jgsogo commented Mar 27, 2023

Approving from my side.

Pinging @mhillenbrand one more, and @mbhealy time to take a look.

Friendly reminder, @mhillenbrand @mbhealy ☺️

Copy link
Contributor

@mhillenbrand mhillenbrand left a comment

Choose a reason for hiding this comment

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

This looks mostly good to me, with a few small nits & suggestions.

include/Payload/Payload.h Show resolved Hide resolved
lib/Payload/ZipPayload/ZipPayload.cpp Outdated Show resolved Hide resolved
lib/Payload/ZipPayload/ZipPayload.cpp Outdated Show resolved Hide resolved
test/unittest/Payload/PayloadRegistryTest.cpp Show resolved Hide resolved
lib/Payload/ZipPayload/ZipPayload.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@mbhealy mbhealy left a comment

Choose a reason for hiding this comment

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

Just one nit

@mbhealy
Copy link
Contributor

mbhealy commented Mar 27, 2023

For some reason GitHub is not allowing me to add my one comment to the file:
Line 42 of lib/Payload/CMakeLists.txt
The comment should be regarding Payloads.inc, not Targets.inc.

1 similar comment
@mbhealy
Copy link
Contributor

mbhealy commented Mar 27, 2023

For some reason GitHub is not allowing me to add my one comment to the file:
Line 42 of lib/Payload/CMakeLists.txt
The comment should be regarding Payloads.inc, not Targets.inc.

jgsogo and others added 2 commits March 27, 2023 15:30
Co-authored-by: Marius Hillenbrand <marius.hillenbrand@ibm.com>
@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

1 similar comment
@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Copy link
Contributor

@mhillenbrand mhillenbrand left a comment

Choose a reason for hiding this comment

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

LGTM.

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.

None yet

4 participants