Update .gitignore when generating templates #137

viveksyngh · 2017-10-03T13:40:25Z

… present .gitignore

Signed-off-by: Vivek Singh [email protected]

Description

Motivation and Context

I have raised an issue to propose this change (required)

How Has This Been Tested?

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I've read the CONTRIBUTION guide
I have signed-off my commits with git commit -s
I have added tests to cover my changes.
All new and existing tests passed.

viveksyngh · 2017-10-03T13:52:50Z

#127

alexellis · 2017-10-03T15:16:34Z

How Has This Been Tested?

This needs to be filled out.

alexellis · 2017-10-03T15:17:19Z

commands/new_function.go

@@ -85,6 +85,8 @@ the "Dockerfile" lang type in your YAML file.
 		fmt.Printf("Folder: %s created.\n", functionName)
 	}

+	createORUpdateGitignore()


Probably could be: updateGitignore - add a comment that it creates it if non-existant

I have updated the function name.

alexellis · 2017-10-03T15:17:43Z

commands/new_function.go

+func createORUpdateGitignore() {
+	FILES_TO_IGNORE := []string{"template", "build"}
+
+	f, err := os.OpenFile(".gitignore", os.O_RDWR|os.O_CREATE, 0644)


does this need an os.Stat to check for the file before opening?

It will create file if it is not present otherwise it will open it with read-write

alexellis · 2017-10-03T15:18:28Z

commands/new_function.go

+	return false
+}
+
+func createORUpdateGitignore() {


Would be better if this returned an error instead of printing out to fmt. Then we can print in one place.

updated and pushed the changes.

open-derek · 2017-10-03T18:04:28Z

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.

viveksyngh · 2017-10-03T18:20:09Z

I have tested this with 2 scenario ..
1.) Created new function when .gitignore file is not present in the current directory
2.) Created a new function when .gitignore file is already present and having some files added

Any suggestion for testcases ?

alexellis · 2017-10-03T20:55:01Z

commands/new_function.go

-			lines := strings.Split(string_content, "\n")
+	if err != nil {
+		return err
+	}


Looks much cleaner without all the nested if statements. 👍

alexellis · 2017-10-04T11:01:14Z

commands/new_function.go

+	return false
+}
+
+func updateGitignore() (err error) {


While we may not be able to test the file editing easily, what do you think about extracting a method that does the transform and testing that independently? This would be good for regression.

So that extracted functions will only take the file content, updates the content and returns it back ? I should write test for that function ?

Sounds like a good approach. That way the function only does 1 thing and can be tested without worrying about editing on disk.

Okay .. I will do that. In which file am I supposed to write test and which library to use for testing ?

Here's an example - https://github.com/openfaas/faas-cli/blob/master/commands/fetch_templates_test.go

Updated and pushed suggested changes.

alexellis

Please could you rebase/squash all these commits into one?

viveksyngh · 2017-10-05T08:11:55Z

I tried doing that .. but it is increasing the number of commit. Can you guide me on this ?

I squashed the commits in local .. when I took pull and pushed the branch to remote it increased the number of commits. Should push only the commit on which I have squashed all other commits ?

viveksyngh · 2017-10-05T09:12:40Z

@alexellis Never mind I got it.

viveksyngh · 2017-10-06T09:20:58Z

I have made the requested changes and squashed all commits into a single commit.

alexellis · 2017-10-06T17:58:53Z

Thank you for that 👍 @viveksyngh I think we should test this on a Windows machine or VM before merging. Can you do that or should we call someone in to the thread?

viveksyngh · 2017-10-06T21:08:09Z

I can do that. I will setup a windows VM and development environment in window and do the testing.

viveksyngh · 2017-10-07T17:55:10Z

@alexellis I have tested it in windows.. It is working fine. Please find the attached gif.

You can also have look at this video.

http://recordit.co/ekkoOtM4PE

viveksyngh · 2017-10-15T08:10:44Z

@alexellis Please review the test result from windows environment that I have attached in previous reply. Please Let me know if further testing is required for windows environment.

alexellis · 2017-10-23T16:28:11Z

commands/update_gitignore.go

+func updateContent(content string) (updated_content string) {
+	// append files to ignore to file content if it is not already ignored
+
+	FILES_TO_IGNORE := []string{"template", "build"}


Please case this as "filesToIgnore" or "ignore"

alexellis

Changes requested, otherwise we need to merge this soon, so please get in touch if you need help.

open-derek · 2017-10-23T16:37:18Z

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.

… present .gitignore Signed-off-by: Vivek Singh <[email protected]> changed function name to updateGitignore, return error from function and print at one place Signed-off-by: Vivek Singh <[email protected]> extracted update function, placed all update .gitignore func in one file added testcases for updateContent func Signed-off-by: Vivek Singh <[email protected]> extracted update function, placed all update .gitignore func in one file added testcases for updateContent func Signed-off-by: Vivek Singh <[email protected]> added testcases for updateContent func changes FILES_TO_IGNORE variable to filesToIgnore Signed-off-by: Vivek Singh <[email protected]> changes FILES_TO_IGNORE variable to filesToIgnore

viveksyngh · 2017-10-23T16:58:24Z

@alexellis I have made the requested changes and updated the PR. Please review it.

alexellis · 2017-11-06T16:10:15Z

Thank you @viveksyngh, apologies for the delay. Are you also on Slack? It's a good place to ping someone for merge/review.

viveksyngh · 2017-11-06T16:17:22Z

Yes I am on slack. How can I join the slack channel ?

alexellis reviewed Oct 3, 2017

View reviewed changes

open-derek bot added the no-dco label Oct 3, 2017

alexellis reviewed Oct 3, 2017

View reviewed changes

alexellis reviewed Oct 4, 2017

View reviewed changes

alexellis changed the title ~~create .gitingore file if does not exists otherwise append to already…~~ Update .gitignore when generating templates Oct 4, 2017

alexellis requested changes Oct 5, 2017

View reviewed changes

viveksyngh force-pushed the update_gitignore branch from 61fa096 to 72061f5 Compare October 5, 2017 09:10

open-derek bot removed the no-dco label Oct 5, 2017

alexellis added the hacktoberfest label Oct 6, 2017

alexellis reviewed Oct 23, 2017

View reviewed changes

alexellis requested changes Oct 23, 2017

View reviewed changes

open-derek bot added the no-dco label Oct 23, 2017

viveksyngh force-pushed the update_gitignore branch from bfcca78 to d40dd48 Compare October 23, 2017 16:41

open-derek bot removed the no-dco label Oct 23, 2017

alexellis force-pushed the master branch from da0b406 to 5cd3ea2 Compare November 3, 2017 13:05

alexellis merged commit 16f019b into openfaas:master Nov 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update .gitignore when generating templates #137

Update .gitignore when generating templates #137

viveksyngh commented Oct 3, 2017 •

edited

Loading

viveksyngh commented Oct 3, 2017 •

edited

Loading

alexellis commented Oct 3, 2017

alexellis Oct 3, 2017

viveksyngh Oct 3, 2017

alexellis Oct 3, 2017

viveksyngh Oct 3, 2017

alexellis Oct 3, 2017

viveksyngh Oct 3, 2017

open-derek bot commented Oct 3, 2017

viveksyngh commented Oct 3, 2017 •

edited

Loading

alexellis Oct 3, 2017

alexellis Oct 4, 2017

viveksyngh Oct 4, 2017

alexellis Oct 4, 2017

viveksyngh Oct 4, 2017

alexellis Oct 4, 2017

viveksyngh Oct 5, 2017

alexellis left a comment

viveksyngh commented Oct 5, 2017 •

edited

Loading

viveksyngh commented Oct 5, 2017

viveksyngh commented Oct 6, 2017

alexellis commented Oct 6, 2017 •

edited

Loading

viveksyngh commented Oct 6, 2017

viveksyngh commented Oct 7, 2017 •

edited

Loading

viveksyngh commented Oct 15, 2017

alexellis Oct 23, 2017 •

edited

Loading

alexellis left a comment

open-derek bot commented Oct 23, 2017

viveksyngh commented Oct 23, 2017

alexellis commented Nov 6, 2017

viveksyngh commented Nov 6, 2017

Update .gitignore when generating templates #137

Update .gitignore when generating templates #137

Conversation

viveksyngh commented Oct 3, 2017 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

viveksyngh commented Oct 3, 2017 • edited Loading

alexellis commented Oct 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

open-derek bot commented Oct 3, 2017

viveksyngh commented Oct 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexellis left a comment

Choose a reason for hiding this comment

viveksyngh commented Oct 5, 2017 • edited Loading

viveksyngh commented Oct 5, 2017

viveksyngh commented Oct 6, 2017

alexellis commented Oct 6, 2017 • edited Loading

viveksyngh commented Oct 6, 2017

viveksyngh commented Oct 7, 2017 • edited Loading

viveksyngh commented Oct 15, 2017

alexellis Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

alexellis left a comment

Choose a reason for hiding this comment

open-derek bot commented Oct 23, 2017

viveksyngh commented Oct 23, 2017

alexellis commented Nov 6, 2017

viveksyngh commented Nov 6, 2017

viveksyngh commented Oct 3, 2017 •

edited

Loading

viveksyngh commented Oct 3, 2017 •

edited

Loading

viveksyngh commented Oct 3, 2017 •

edited

Loading

viveksyngh commented Oct 5, 2017 •

edited

Loading

alexellis commented Oct 6, 2017 •

edited

Loading

viveksyngh commented Oct 7, 2017 •

edited

Loading

alexellis Oct 23, 2017 •

edited

Loading