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

[ADXT-783] Made Runner interface for Remote and Local runners #1288

Merged
merged 21 commits into from
Dec 26, 2024

Conversation

CelianR
Copy link
Contributor

@CelianR CelianR commented Dec 11, 2024

What does this PR do?

Did an interface that gathers remote and local runners.

Notes

  • microvm/runner file has been removed since it is useless now
  • The environment is passed additionally to commands, otherwise there is no shell expansion to variables we define in the arguments
  • MoveRemoteFile command has been updated since ' were not escaping the file paths, changed to double quotes
  • A second PR will come to refactor filemanager functions and command.Args
  • Datadog agent pipeline

Which scenarios this will impact?

Motivation

Additional Notes

  • Tested with local podman example

@CelianR CelianR self-assigned this Dec 11, 2024
@CelianR CelianR force-pushed the celian/refactor-runner-adxt-783 branch from 161b08b to ab0b21a Compare December 13, 2024 14:45
components/command/osCommand.go Outdated Show resolved Hide resolved
components/command/runner.go Outdated Show resolved Hide resolved
@CelianR CelianR marked this pull request as ready for review December 18, 2024 13:33
@CelianR CelianR requested review from a team as code owners December 18, 2024 13:33
components/command/osCommand.go Outdated Show resolved Hide resolved
Comment on lines +25 to +27
// Only used for local commands
LocalAssetPaths pulumi.StringArrayInput
LocalDir pulumi.StringInput
Copy link
Contributor

@pducolin pducolin Dec 18, 2024

Choose a reason for hiding this comment

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

💬 suggestion
For this, you could have a LocalArguments interface, that only a new struct LocalArgs implement, and cast Args to LocalArgs here, failing if the cast fails.

This would be something like

type Args struct {
	Create                   pulumi.StringInput
	Update                   pulumi.StringInput
	Delete                   pulumi.StringInput
	Triggers                 pulumi.ArrayInput
	Stdin                    pulumi.StringPtrInput
	Environment              pulumi.StringMap
	RequirePasswordFromStdin bool
	Sudo                     bool
}

type LocalArguments interface {
	LocalAssetPaths() pulumi.StringArrayInput
	LocalDir() 		       pulumi.StringInput
}

type LocalArgs struct {
	Args
	LocalAssetPaths pulumi.StringArrayInput
	LocalDir        pulumi.StringInput  
}

var _ LocalArgument = *LocalArgs{}

func (args *LocalArgs) LocalAssetPaths() pulumi.StringArrayInput {
	return args.LocalAssetPaths
}

func (args *LocalArgs) LocalDir() pulumi.StringInput {
	return args.LocalDir
}

func (args *Args) toLocalCommandArgs(config RunnerConfiguration, osCommand OSCommand) (*local.CommandArgs, error) {
	if localArgs, ok := args.(LocalArgs); !ok {
		return nil, fmt.Errorf("can convert only local args")
	} 
	return &local.CommandArgs{
		Create:   osCommand.Build
		Create:      osCommand.BuildCommandString(args.Create, args.Environment, args.Sudo, args.RequirePasswordFromStdin, config.user),
		Update:      osCommand.BuildCommandString(args.Update, args.Environment, args.Sudo, args.RequirePasswordFromStdin, config.user),
		Delete:      osCommand.BuildCommandString(args.Delete, args.Environment, args.Sudo, args.RequirePasswordFromStdin, config.user),
		Environment: args.Environment,
		Triggers:    args.Triggers,
		Stdin:       args.Stdin,
		AssetPaths:  localArgs.LocalAssetPaths,
		Dir:         localArgs.LocalDir,
	}, nil
}

Don't know if you can possibly define toLocalCommandArgs on LocalArgs rather than on Args, I think you showed me yesterday you could not

components/command/runner.go Outdated Show resolved Hide resolved
resources/local/podman/vm.go Show resolved Hide resolved
@pducolin
Copy link
Contributor

Passing all tests in datadog-agent DataDog/datadog-agent#32459

@pducolin
Copy link
Contributor

/merge

@dd-devflow
Copy link

dd-devflow bot commented Dec 26, 2024

Devflow running: /merge

View all feedbacks in Devflow UI.


2024-12-26 09:39:38 UTC ℹ️ MergeQueue: pull request added to the queue

The median merge time in main is 3m.


2024-12-26 09:41:51 UTC ℹ️ MergeQueue: This merge request was merged

@dd-mergequeue dd-mergequeue bot merged commit 1a62db6 into main Dec 26, 2024
8 checks passed
@dd-mergequeue dd-mergequeue bot deleted the celian/refactor-runner-adxt-783 branch December 26, 2024 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants