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 PlatformProperties to client.Action #528

Open
akshay-verma opened this issue Jan 18, 2024 · 3 comments
Open

Add PlatformProperties to client.Action #528

akshay-verma opened this issue Jan 18, 2024 · 3 comments

Comments

@akshay-verma
Copy link

akshay-verma commented Jan 18, 2024

This needs two changes -

  1. Update the Action struct to add PlatformProperties map[string]string -
type Action struct {
	// Args are the command-line arguments to start the process. The first argument is the process
	// name, and the rest are its arguments.
	Args []string
	// EnvVars are the variables to add to the process's environment.
	EnvVars map[string]string
	// InputRoot and InputFiles contain the details of the input tree, in remote execution format.
	// They should normally be constructed through the PackageTree function.
	InputRoot  digest.Digest
	InputFiles map[digest.Digest][]byte
	// OutputFiles is a list of output files requested (full paths).
	OutputFiles []string
	// OutputDirs is a list of output directories requested (full paths).
	OutputDirs []string
	// Docker image is a docker:// URL to the docker image in which execution will take place.
	DockerImage string
	// Timeout is the maximum execution time for the action. Note that it's not an overall timeout on
	// the process, since there may be additional time for transferring files, waiting for a worker to
	// become available, or other overhead.
	//
	// If 0, the server's default timeout is used.
	Timeout time.Duration
	// DoNotCache, if true, indicates that the result of this action should never be cached. It
	// implies SkipCache.
	DoNotCache bool
	// SkipCache, if true, indicates that this action should be executed even if there is a copy of
	// its result in the action cache that could be used instead.
	SkipCache bool
+
+  // PlatformProperties are the properties to add to the command's Platform properties
+  PlatformProperties map[string]string
+
}
  1. Update PrepAction function to add this piece of code -
for name, val := range ac.PlatformProperties {
		reAc.Platform.Properties = append(cmd.Platform.Properties, &repb.Platform_Property{
			Name: name, Value: val,
		})
	}

Usage -

ac := client.Action{
	Args: []string{"..."},
	InputRoot: "...",
	DockerImage: "...",
	PlatformProperties: map[string]string{
		"prop1": "value1",
		"prop2": "value2"
	}
}

Does this change make sense? I think the DockerImage variable in Action struct could have been part of the generic platform properties to begin with.

@mrahs
Copy link
Collaborator

mrahs commented Feb 22, 2024

Hmm, Action seems to be a convenient struct that is used to construct a Command:

func buildCommand(ac *Action) *repb.Command {
cmd := &repb.Command{
Arguments: ac.Args,
// Do not use OutputFiles and OutputDirs directly from the Action, as we need to sort them which
// implies modification.
OutputFiles: make([]string, len(ac.OutputFiles)),
OutputDirectories: make([]string, len(ac.OutputDirs)),
Platform: &repb.Platform{
Properties: []*repb.Platform_Property{{Name: containerImagePropertyName, Value: ac.DockerImage}},
},
}
copy(cmd.OutputFiles, ac.OutputFiles)
copy(cmd.OutputDirectories, ac.OutputDirs)
sort.Strings(cmd.OutputFiles)
sort.Strings(cmd.OutputDirectories)
for name, val := range ac.EnvVars {
cmd.EnvironmentVariables = append(cmd.EnvironmentVariables, &repb.Command_EnvironmentVariable{Name: name, Value: val})
}
sort.Slice(cmd.EnvironmentVariables, func(i, j int) bool { return cmd.EnvironmentVariables[i].Name < cmd.EnvironmentVariables[j].Name })
return cmd
}

The docker image is, in fact, specified as a platform property on the command struct.

Is there a specific use-case that benefits from generalizing Action or can that be solved by using Command directly instead?

@akshay-verma
Copy link
Author

akshay-verma commented Feb 24, 2024

So from what I could grok neither Action nor Command allows adding PlatformProperties other than DockerImage. My use case is I need to set some additional platform properties before executing the action but I can't do that, and that means I can't use these methods and need to write my own which is unfortunate.

@mrahs
Copy link
Collaborator

mrahs commented Mar 4, 2024

I see. The request here is to expose the properties to allow clients to add desired ones. I'm happy to review PRs for this. I cannot promise posting one myself, unfortunately.

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

No branches or pull requests

2 participants