Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

typos + broken links in documentation #20

Merged
merged 6 commits into from
Nov 16, 2019
Merged

typos + broken links in documentation #20

merged 6 commits into from
Nov 16, 2019

Conversation

pablito-perez
Copy link
Contributor

@pablito-perez pablito-perez commented Nov 15, 2019 •
edited
Loading

fix a couple of typos, update to latest version

w3st3ry
w3st3ry suggested changes Nov 15, 2019
View reviewed changes
Copy link
Contributor

@w3st3ry w3st3ry left a comment

Choose a reason for hiding this comment

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

Could you update the version name in the Makefile too?

@pablito-perez
Copy link
Contributor Author

changed version in Makefile
would like to make this value a parameter to be received when generating the install script and when building with make

@w3st3ry
Copy link
Contributor

w3st3ry commented Nov 15, 2019 •
edited
Loading

changed version in Makefile
would like to make this value a parameter to be received when generating the install script and when building with make

What about directly get the value from the latest git tag?
Something like git describe --tags $(git rev-list --tags --max-count=1)

w3st3ry
w3st3ry suggested changes Nov 15, 2019
View reviewed changes
w3st3ry
w3st3ry reviewed Nov 15, 2019
View reviewed changes
Copy link
Contributor

@w3st3ry w3st3ry left a comment

Choose a reason for hiding this comment

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

Why delete the LICENSE file from github.com/wI2L/fizz?

@pablito-perez
typos + broken links in documentation
7d9a5ff 
Signed-off-by: Pablo Pérez Schröder <2639770+nomonamo@users.noreply.github.com>
@pablito-perez
up version in Makefile
42b77f9 
Signed-off-by: Pablo Pérez Schröder <2639770+nomonamo@users.noreply.github.com>
@pablito-perez
Copy link
Contributor Author

LICENSE file in fizz is a mistake :/

@pablito-perez
revert file removal
fca1b71 
Signed-off-by: Pablo Pérez Schröder <2639770+nomonamo@users.noreply.github.com>
@pablito-perez
don't hardcode software version, take it from latest git tag
7f0ba3b 
Signed-off-by: Pablo Pérez Schröder <2639770+nomonamo@users.noreply.github.com>
Alkorin
Alkorin reviewed Nov 15, 2019
View reviewed changes
hack/generate-install-script.sh Outdated
@@ -35,4 +35,8 @@ write_block "sql/schema.sql" sql/schema.sql
echo "### TEMPLATES" >> $dst
write_block "templates/hello-world-now.yaml" ./examples/templates/hello-world-now.yaml

version=`git describe --tags $(git rev-list --tags --max-count=1)`
sed -i.bak "s/DOCKER_TAG/$version/" ./$dst
Copy link
Member

Choose a reason for hiding this comment

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

No need to create a backup file if it is deleted just after ;)

Suggested change
sed -i.bak "s/DOCKER_TAG/$version/" ./$dst
sed -i "s/DOCKER_TAG/$version/" ./$dst

Copy link
Contributor Author

@pablito-perez pablito-perez Nov 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

incredible as it may sound, it's there for compatibility with mac os :|
https://stackoverflow.com/questions/5694228/sed-in-place-flag-that-works-both-on-mac-bsd-and-linux

Alkorin
Alkorin reviewed Nov 15, 2019
View reviewed changes
hack/generate-install-script.sh Outdated
@@ -35,4 +35,8 @@ write_block "sql/schema.sql" sql/schema.sql
echo "### TEMPLATES" >> $dst
write_block "templates/hello-world-now.yaml" ./examples/templates/hello-world-now.yaml

version=`git describe --tags $(git rev-list --tags --max-count=1)`
sed -i.bak "s/DOCKER_TAG/$version/" ./$dst
rm $dst.bak
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
rm $dst.bak

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a comment in the code, for clarity

Copy link
Member

Choose a reason for hiding this comment

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

The -i option is not POSIX compliant. So you can't expect it to work everywhere.

https://pubs.opengroup.org/onlinepubs/9699919799/utilities/sed.html

See #22

w3st3ry
w3st3ry approved these changes Nov 15, 2019
View reviewed changes
Copy link
Contributor

@w3st3ry w3st3ry left a comment

Choose a reason for hiding this comment

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

Align typo on Makefile should be fixed. Otherwise LGFM 👍

@pablito-perez
fix alignment
24dc667 
Signed-off-by: Pablo Pérez Schröder <2639770+nomonamo@users.noreply.github.com>
@pablito-perez
add explanation for weird sed
69f4617 
Signed-off-by: Pablo Pérez Schröder <2639770+nomonamo@users.noreply.github.com>
@pablito-perez pablito-perez merged commit b255c42 into master Nov 16, 2019
@w3st3ry w3st3ry deleted the doc branch November 16, 2019 18:16
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@Alkorin Alkorin Alkorin left review comments

@w3st3ry w3st3ry w3st3ry approved these changes

@loopfz loopfz Awaiting requested review from loopfz loopfz is a code owner

@rbeuque74 rbeuque74 Awaiting requested review from rbeuque74 rbeuque74 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pablito-perez @w3st3ry @Alkorin