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 suitable comments for GoDoc #84

Merged
merged 1 commit into from
Jul 6, 2019

Conversation

ayushjn20
Copy link
Collaborator

Resolves #76

@apoorvam
Copy link
Collaborator

@ayushjn20 Build is failing with this. Can you fix it so we can merge?

@ayushjn20
Copy link
Collaborator Author

Got it. I opened a PR from my own fork, that's why the continuous-integration/travis-ci/pr is failing on snap login, but the continuous-integration/travis-ci/push build is passing.
I have pushed a branch on the original repo too, the build is passing. Kindly check here
@agentmilindu @apoorvam I think we are good to go with this PR.

Copy link
Contributor

@agentmilindu agentmilindu left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -129,7 +134,8 @@ func process(configs *config.Configs, s *docker.Step, wg *sync.WaitGroup, args [
}
}

func passArgs(s *docker.Step, args *[]string) error {
// PassArgs replaces argument variables,of the form '`$d`', where d is a number, with dth argument.
func PassArgs(s *docker.Step, args *[]string) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you want to expose these functions outside package? Its used only within the package.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah! If dunner is going to be used as a library, then the method that passes the arguments to the commands should be exposed.

@agentmilindu agentmilindu merged commit 9bdb3fa into leopardslab:develop Jul 6, 2019
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