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 a DryRun() method to agent #133

Merged
merged 7 commits into from
Jun 14, 2022
Merged

Add a DryRun() method to agent #133

merged 7 commits into from
Jun 14, 2022

Conversation

mkcp
Copy link
Contributor

@mkcp mkcp commented Jun 13, 2022

The original goal of this refactor was to swap out a different agent with different state for running the dryrun. However, i made the classic mistake of thinking returning an interface was a good idea -- go ensures it isn't! Instead, the simpler route is to reuse the same agent, but branch off early in the Run function to instead do DryRun things.

The benefits of this is that we're no longer checking in with the agent state for the dryrun flag throughout the Run function. It all reads a lot more linear. The tradeoff is we a bit more code repetition now.

For the most part, dryrun steps don't rely on things to occur in the main run. However, there is a workaround for copiers containing the filepath in their ID. We writing a glob character * to Agent.tmpDir dryrun so that way those IDs are valid and can be copied into filters in a real run with a specific filepath.

@mkcp mkcp requested a review from a team June 13, 2022 22:48
@mkcp mkcp self-assigned this Jun 13, 2022
Copy link
Collaborator

@nwchandler nwchandler left a comment

Choose a reason for hiding this comment

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

I left a couple small suggestions, but overall, it looks good to me.

agent/agent.go Outdated Show resolved Hide resolved

// Running healthchecks for products. We don't want to stop if any fail though.
a.l.Info("Checking product availability")
if errProductChecks := a.CheckAvailable(); errProductChecks != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if dryrun should just check whether the required clients are installed using the products' ClientCheck commands instead of also running a healthcheck. I could see it being a bit unexpected to a user if dryrun made any outbound calls.

I know this is existing behavior, so this PR might not be the place to change it, but seeing the CheckAvailable call here reminded me of it.

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 agree that issuing http reqs does break the contract a little bit, even if it's helpful. Maybe this is our first usecase for multiple commands in hcdiag?

I could see adding:

hcdiag validate
hcdiag dryrun
hcdiag run

agent/agent.go Outdated Show resolved Hide resolved
@nwchandler
Copy link
Collaborator

I also think we can get rid of the TODO on agent/line 21 because with this PR, it's TO-DONE. 😄

mkcp and others added 4 commits June 14, 2022 11:31
Co-authored-by: Nick Chandler <nwchandler@users.noreply.github.com>
Co-authored-by: Nick Chandler <nwchandler@users.noreply.github.com>
The dependency github.com/shirou/gopsutil/v3 is updated to address a build-time deprecation warning in MacOS Monterey (12.0) and beyond.

The dependency github.com/stretchr/testify has had a patch update applied.

The library github.com/hashicorp/go-multierror is no longer used and has been removed.
@mkcp mkcp merged commit e771daa into main Jun 14, 2022
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

2 participants