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

Stop pull and build docker image for action multiple times #79

Merged
merged 1 commit into from
Aug 16, 2019

Conversation

TingluoHuang
Copy link
Member

@bryanmacfarlane
Copy link
Member

@jclem for review

@bryanmacfarlane
Copy link
Member

@thboop - can you have a look?

@bryanmacfarlane
Copy link
Member

Thanks for adding L0 tests ...

Copy link
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.

Minor thoughts + test updates, then it looks good

public string WorkingDirectory { get; set; }
}

public class ActionContainer
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could consider making this a Container class and having ContainerSetupInfo have a reference to that class and and its list of StepId's. If we felt like we wanted to preserve the hard typing for ActionContainers, we can do that by creating a Container class, then an ActionContainer class that inherits from it with its new field of ActionRepository.

It might make maintenance easier down the line.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

Assert.Equal((steps[0].Data as ContainerSetupInfo).StepId, actionId);
Assert.Equal((steps[0].Data as ContainerSetupInfo).WorkingDirectory, Path.Combine(Path.GetDirectoryName(watermarkFile), "master"));
Assert.Equal((steps[0].Data as ContainerSetupInfo).Dockerfile, Path.Combine(Path.GetDirectoryName(watermarkFile), "master", "images/cli", "Dockerfile"));
Assert.Equal((steps[0].Data as ActionsContainerSetupInfo).StepIds[0], actionId);
Copy link
Collaborator

@thboop thboop Aug 15, 2019

Choose a reason for hiding this comment

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

I think you want ContainerSetupInfo, I don't think the ActionsContainerSetupInfo class exists

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed.

}
else
{
setupInfo.ActionRepository = $"{repositoryReference.Name}@{repositoryReference.Ref}";
Copy link
Collaborator

@thboop thboop Aug 15, 2019

Choose a reason for hiding this comment

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

I really like the decision to include ref here and a few lines up, matches nicely with how docker does things!

setupInfo.WorkingDirectory = destDirectory;
return setupInfo;

// return new ContainerSetupInfo(dockerFileLowerCase, destDirectory);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We may just want to remove this, we can always see it in the git history

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed.


Assert.Equal((steps[4].Data as ActionsContainerSetupInfo).StepIds[0], actionId8);
Assert.Equal((steps[4].Data as ActionsContainerSetupInfo).WorkingDirectory, Path.Combine(Path.GetDirectoryName(watermarkFile), "master"));
Assert.Equal((steps[4].Data as ActionsContainerSetupInfo).Dockerfile, Path.Combine(Path.GetDirectoryName(watermarkFile), "master", "images/cli", "Dockerfile"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I really like these test cases 👍

@TingluoHuang TingluoHuang merged commit 1a56380 into master Aug 16, 2019
@TingluoHuang TingluoHuang deleted the users/tihuang/lessnodes branch August 16, 2019 01:47
volker-raschek pushed a commit to dedalus-cis4u/github-runner-role that referenced this pull request Jul 25, 2022
Ability to use custom GitHub Actions Runner forks
jsoref pushed a commit to jsoref/actions-runner that referenced this pull request Oct 7, 2022
* Default value for choice input workflow_dispatch
* Disable inputs context for workflow_dispatch, still no sign on github.com
* Add Test
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

4 participants