Skip to content

expand job container on runner side#101

Merged
ericsciple merged 4 commits into
masterfrom
users/ersciple/m158container
Sep 10, 2019
Merged

expand job container on runner side#101
ericsciple merged 4 commits into
masterfrom
users/ersciple/m158container

Conversation

@ericsciple
Copy link
Copy Markdown
Collaborator

@ericsciple ericsciple commented Sep 7, 2019

@bryanmacfarlane
Copy link
Copy Markdown
Member

Please update description with link to workitem on the team board.

ContainerInfo Container { get; }
List<ContainerInfo> SidecarContainers { get; }
ContainerInfo Container { get; set; }
List<ContainerInfo> ServiceContainers { get; }
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

converged on "Services" everywhere, previously mix of "Sidecar" and "Services" (due to history, rename)

public Dictionary<string, VariableValue> TaskVariables { get; set; }
public Dictionary<string, string> Inputs { get; set; }
public Dictionary<String, PipelineContextData> Context { get; set; }
public DictionaryContextData Context { get; set; } = new DictionaryContextData();
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

simplified the type, multiple places

}

public ContainerInfo(IHostContext hostContext, Pipelines.ContainerResource container, Boolean isJobContainer = true)
public ContainerInfo(IHostContext hostContext, Pipelines.JobContainer container, bool isJobContainer = true, string networkAlias = null)
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

this is one of the last few things that used "Resources". i switched to a simple data object "JobContainer" defined in the web api

ExpressionValues["runner"] = new RunnerContext();
ExpressionValues["job"] = new JobContext();

if (!ExpressionValues.ContainsKey("github"))
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

bye bye back compat code

// Prepend Path
PrependPath = new List<string>();

// Docker (JobContainer)
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

this has to be evaluated now during job initialize

}
}

// Evaluate the job container
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

here is where the container evaluation happens now

context.SetRunnerContext("workspace", Path.Combine(_workDirectory, trackingConfig.PipelineDirectory));
context.SetGitHubContext("workspace", Path.Combine(_workDirectory, trackingConfig.WorkspaceDirectory));

// Build up 3 lists of steps, pre-job, job, post-job
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

these two lines just moved down a little to be grouped with the code that is relevant to this

}
}

public IDictionary<String, String> JobSidecarContainers
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

i wanted to remove this property to make sure i fixed everywhere

@ericsciple ericsciple marked this pull request as ready for review September 9, 2019 04:28
var serviceContainer = pair.Value;
jobContext.ServiceContainers.Add(new Container.ContainerInfo(HostContext, serviceContainer, false, networkAlias));
}
}
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

ExpressionValues["github"] = githubContext;
githubContext[pair.Key] = pair.Value;
}
ExpressionValues["github"] = githubContext;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this is much cleaner, nice

githubAccessToken = repoEndpoint.Authorization.Parameters["accessToken"];
githubContext["token"] = new StringContextData(githubAccessToken);
var base64EncodingToken = Convert.ToBase64String(Encoding.UTF8.GetBytes($"x-access-token:{githubAccessToken}"));
HostContext.SecretMasker.AddValue(base64EncodingToken);
Copy link
Copy Markdown
Collaborator

@thboop thboop Sep 10, 2019

Choose a reason for hiding this comment

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

Did you mean to pull out the Secret Masking of the token? (Above as well)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The rest of this looks good to me! I'd like to just clarify this part

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@TingluoHuang we dont need this anymore now that you added the base64 encoders, right?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yes, we don't really need this.

@ericsciple ericsciple force-pushed the users/ersciple/m158container branch from 331211d to ceee866 Compare September 10, 2019 17:49
Comment thread src/Runner.Worker/ExecutionContext.cs Outdated
Copy link
Copy Markdown
Collaborator

@thboop thboop left a comment

Choose a reason for hiding this comment

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

LGTM 🥇

@ericsciple ericsciple merged commit c73f40f into master Sep 10, 2019
@ericsciple ericsciple deleted the users/ersciple/m158container branch September 10, 2019 19:23
m_maskHints = null;
}
else
else if (m_maskHints != null)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

good fix. 😄

jsoref pushed a commit to jsoref/actions-runner that referenced this pull request Oct 7, 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.

4 participants