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

AGENT-848: add node-joiner cli tool main #7958

Merged
merged 1 commit into from Feb 1, 2024

Conversation

andfasano
Copy link
Contributor

This patch introduces the node-joiner tool main with (empty) commands entry points. Since the commands will reuse the existing assets, a new AssetsFetcher interface and struct has been introduced to allow sharing the same asset fetcher code already used by the create and agent commands in the default installer.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jan 29, 2024
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jan 29, 2024

@andfasano: This pull request references AGENT-848 which is a valid jira issue.

In response to this:

This patch introduces the node-joiner tool main with (empty) commands entry points. Since the commands will reuse the existing assets, a new AssetsFetcher interface and struct has been introduced to allow sharing the same asset fetcher code already used by the create and agent commands in the default installer.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@andfasano
Copy link
Contributor Author

cc @rwsu

@openshift-ci openshift-ci bot requested review from r4f4 and sadasu January 29, 2024 12:00
Copy link
Contributor

@rwsu rwsu left a comment

Choose a reason for hiding this comment

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

Besides the lint and fmt issues, this lgtm.

pkg/asset/store/assetsfetcher.go Outdated Show resolved Hide resolved
pkg/asset/store/assetsfetcher.go Outdated Show resolved Hide resolved
pkg/asset/store/assetsfetcher.go Outdated Show resolved Hide resolved
pkg/asset/store/assetsfetcher.go Outdated Show resolved Hide resolved
Comment on lines +15 to +20
type fetcher struct {
storeDir string
}
Copy link
Contributor

Choose a reason for hiding this comment

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

would it make sense to have storeImpl implement AssetFetcher? It's an existing struct that contains this data structure.

type storeImpl struct {
directory string
assets map[reflect.Type]*assetState
stateFileAssets map[string]json.RawMessage
fileFetcher asset.FileFetcher

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 also initially had a look at it, but given the amount of stuff/responsabilities already implemented by storeImpl my preference was for a more isolated/separated approach, to avoid overload it too much.

Introduces no-joiner tool main with (empty) commands entry points
Copy link
Contributor

openshift-ci bot commented Jan 31, 2024

@andfasano: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Copy link
Contributor

@rwsu rwsu left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 1, 2024
@sadasu
Copy link
Contributor

sadasu commented Feb 1, 2024

/approve

Copy link
Contributor

openshift-ci bot commented Feb 1, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sadasu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 1, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 68a1f91 into openshift:master Feb 1, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants