Skip to content
This repository has been archived by the owner on Feb 2, 2021. It is now read-only.

Added check for template file #197

Merged
merged 1 commit into from
Mar 6, 2020
Merged

Conversation

kadern0
Copy link
Contributor

@kadern0 kadern0 commented Mar 2, 2020

Description

Added check to verify template file exists, if not, an informative message is printed.

Fixes #194

How Has This Been Tested?

Run the test from project's root directory:

./bin/ofc-bootstrap create-github-app --root-domain test
Name: OFC elegant leavitt5	Root domain: test	Scheme: https
Launching browser: http://127.0.0.1:30010
Starting local HTTP server on port 30010
Please complete the workflow in the browser to create your GitHub App.
^C

Run the test from outside:

kaderno@kaderno-XPS-13-9360:~/go/src/github.com$ ./ofc-bootstrap/bin/ofc-bootstrap create-github-app --root-domain test
Error: Cannot find the template file. You need to run this command from the root directory of the repository.

Checklist:

I have:

  • checked my changes follow the style of the existing code / OpenFaaS repos
  • updated the documentation and/or roadmap in README.md
  • read the CONTRIBUTION guide
  • signed-off my commits with git commit -s
  • added unit tests

@derek
Copy link

derek bot commented Mar 2, 2020

Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off. That's something we need before your Pull Request can be merged. Please see our contributing guide.
Tip: if you only have one commit so far then run: git commit --amend --signoff and then git push --force.

@derek derek bot removed the no-dco label Mar 2, 2020
Copy link
Contributor

@Waterdrips Waterdrips left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@alexellis alexellis left a comment

Choose a reason for hiding this comment

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

Just a small change please

@@ -64,6 +65,11 @@ func createGitHubAppE(command *cobra.Command, _ []string) error {
"GitHubEvent": fmt.Sprintf("%s://system.%s/github-event", scheme, rootDomain),
}

_, err := os.Stat(path.Join("./pkg/github", "index.html"))
Copy link
Member

Choose a reason for hiding this comment

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

Please can you inline this? if _, err :=

@alexellis
Copy link
Member

Nice work 😁

@kadern0 kadern0 force-pushed the issue-194 branch 2 times, most recently from be72124 to f7fb0d7 Compare March 4, 2020 07:51
@@ -64,6 +65,10 @@ func createGitHubAppE(command *cobra.Command, _ []string) error {
"GitHubEvent": fmt.Sprintf("%s://system.%s/github-event", scheme, rootDomain),
}

if _, err := os.Stat(path.Join("./pkg/github", "index.html")); err != nil {
return fmt.Errorf("Cannot find the template file. You need to run this command from the root directory of the repository.\n ")
Copy link
Member

Choose a reason for hiding this comment

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

Just so that you know, errors in Go cannot start with an uppercase letter or end with a .

Copy link
Member

Choose a reason for hiding this comment

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

Can I suggest this alternative? -> cannot find template "index.html", run this command from the ofc-bootstrap repository

Copy link
Member

@alexellis alexellis left a comment

Choose a reason for hiding this comment

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

One more change, again, just stylistic about how Go projects are usually written.

@@ -64,6 +65,10 @@ func createGitHubAppE(command *cobra.Command, _ []string) error {
"GitHubEvent": fmt.Sprintf("%s://system.%s/github-event", scheme, rootDomain),
}

if _, err := os.Stat(path.Join("./pkg/github", "index.html")); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Spot on 👍

Copy link
Member

@alexellis alexellis left a comment

Choose a reason for hiding this comment

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

I think you need to use ``` backticks for the outer string did this compile ok?

@alexellis
Copy link
Member

You need to build locally before pushing. This is causing a syntax error. Please remove the newline too.


`some "string" goes here`

Signed-off-by: kadern0 <kaderno@gmail.com>
@kadern0
Copy link
Contributor Author

kadern0 commented Mar 4, 2020

That happens when you copy and paste without looking :( my bad!

Copy link
Member

@alexellis alexellis left a comment

Choose a reason for hiding this comment

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

Looks perfect. Hope the feedback was useful? Will merge when CI passes

@alexellis alexellis merged commit 19a7220 into openfaas:master Mar 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Throw user-friendly error for "create-github-app" when template not found
3 participants