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.

Sign up for GitHub

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

Jump to bottom

Append flag doesnt add newline if not present in file #852

Open
Waterdrips opened this issue Dec 17, 2020 · 13 comments · May be fixed by #902
Open

Append flag doesnt add newline if not present in file #852

Waterdrips opened this issue Dec 17, 2020 · 13 comments · May be fixed by #902
Assignees
@kylos101 @Waterdrips
Labels
help wanted

Comments

@Waterdrips
Copy link
Contributor

Waterdrips commented Dec 17, 2020 •
edited

Expected Behaviour

The append flag shuld a function correctly if the file doesn't have a newline at the end

Current Behaviour

When using faas-cli new --append stack.yml i have found that this doesnt append a newline if one is not present in the file

I therefore created a new function and it appended like this

  route53:
    lang: golang-middleware
    handler: ./route53
    image: route53:latest  route54:       # <-- appended to here without newline
    lang: golang-middleware
    handler: ./route54
    image: route54:latest

Possible Solution

check the stack file for a newline, if not present add one?

Steps to Reproduce (for bugs)

  1. faas-cli new --lang golang new -f stack.yml
  2. remove newlines from the stack.yml
  3. faas-cli new --lang golang new2 --append stack.yml
  4. check file, its not added a newline before the func so its squashed the the last line of the first definition

Context

Your Environment

  • FaaS-CLI version ( Full output from: faas-cli version ):
  ___                   _____           ____
 / _ \ _ __   ___ _ __ |  ___|_ _  __ _/ ___|
| | | | '_ \ / _ \ '_ \| |_ / _` |/ _` \___ \
| |_| | |_) |  __/ | | |  _| (_| | (_| |___) |
 \___/| .__/ \___|_| |_|_|  \__,_|\__,_|____/
      |_|

CLI:
 commit:  598336a0cad38a79d5466e6a3a9aebab4fc61ba9
 version: 0.12.21
Error with YAML file
  • Operating System and version (e.g. Linux, Windows, MacOS):
    Linux
@alexellis
Copy link
Member

@Waterdrips I'm going to assign this one to you, can you send a PR and a unit test when you have time.

@alexellis
Copy link
Member

/add label: help wanted

@derek derek bot added the help wanted label Mar 11, 2021
@jantytgat
Copy link

I'm willing to attempt to fix this one.

@alexellis
Copy link
Member

Thanks @jantytgat

@kylos101
Copy link

kylos101 commented Nov 6, 2021

Hi, I'll take a peek at this issue Saturday evening (11/6). I figure it may already be fixed or still need fixin'...either way I'll let you know. 👍

@alexellis
Copy link
Member

Thank you @kylos101 💪

@kylos101
Copy link

kylos101 commented Nov 7, 2021

Hi, it's definitely still an issue, I was able to recreate using the instructions in this issue.

I started a fix last night, and will share a draft later today.

kylos101 added a commit to kylos101/faas-cli that referenced this issue Nov 7, 2021
@kylos101
Fixes openfaas#852 optionally add last newline
9862ae7
@kylos101 kylos101 linked a pull request Nov 7, 2021 that will close this issue
Open
11 tasks
kylos101 added a commit to kylos101/faas-cli that referenced this issue Nov 7, 2021
@kylos101
Fixes openfaas#852 optionally add last newline
04851ac 
Signed-off-by: Kyle Brennan <[email protected]>
@kylos101 kylos101 mentioned this issue Nov 21, 2021
Closed
2 tasks
kylos101 added a commit to kylos101/faas-cli that referenced this issue Nov 21, 2021
@kylos101
Fixes openfaas#852 optionally add last newline
a52cd2b 
Tested with Linux and Windows, by appending many new functions to
a single stack.yml file.

Signed-off-by: Kyle Brennan <[email protected]>
@kylos101
Copy link

Evening 👋 , the 🔧 for this should be all set in #902 , let me know if any changes are needed? 👍 👍

@kylos101
Copy link

kylos101 commented Dec 1, 2021

Hey @alexellis , may I ask for you to take a peek at #902, and let me know if you'd like anything else?

I tested in Linux and Windows, wrote new unit tests (hooray, coverage!), and asserted no existing tests are broken.

@alexellis
Copy link
Member

I did take a look already and commented.

I'll add it to the project view so it doesn't keep getting bumped off my radar.

https://github.com/orgs/openfaas/projects/5

@kylos101
Copy link

kylos101 commented Dec 4, 2021

Hey @alexellis , I might be missing something... 😟

I thought I addressed your comments from the initial review 21 days ago in a52cd2b. Can you share any feedback that I have not addressed / you would like me to do (a link is okay too)? 👍 👍

@kylos101
Copy link

kylos101 commented Dec 5, 2021

Nvm, I get it now, we're working from a Github project - cool. I'll follow-up in #contributors.

kylos101 added a commit to kylos101/faas-cli that referenced this issue Dec 13, 2021
@kylos101
Fixes openfaas#852 optionally add last newline
6436a8f 
Tested with Linux and Windows, by appending many new functions to
a single stack.yml file.

Signed-off-by: Kyle Brennan <[email protected]>
@kylos101
Copy link

Hey @alexellis 👋

This one should be all set now 🥳 🎉

Lemme know if anything else is needed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kylos101 kylos101

@Waterdrips Waterdrips

Labels
help wanted
Projects
OpenFaaS - Dec 2021
Status: No status
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fixes #852 optionally add last newline kylos101/faas-cli
4 participants
@kylos101 @alexellis @jantytgat @Waterdrips