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

docs: add conditional deployment use case to readme #289

Conversation

tprouvot
Copy link
Contributor

What does this pull request contain?

Add conditional deplyment use case in the README, as a solution to solve deployment errors when delta files are empty.

Does this close any currently open issues?

closes #249

Copy link
Owner

@scolladon scolladon left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks for this addition

@codecov
Copy link

codecov bot commented Apr 13, 2022

Codecov Report

Merging #289 (13480a7) into main (f43b8e0) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##              main      #289   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           22        22           
  Lines          628       628           
=========================================
  Hits           628       628           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f43b8e0...13480a7. Read the comment docs.

Copy link
Owner

@scolladon scolladon left a comment

Choose a reason for hiding this comment

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

Few changes we can discuss.

Thanks for the work so far, I think it is very helpful

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@mehdicherf mehdicherf left a comment

Choose a reason for hiding this comment

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

Thank you Thomas!
In addition to @scolladon comments, I would add 2 things:

  1. Add the new section to the Table of Contents
  2. I would put it before the U_se the module in your own node application_ section (that last section is for very specific use-cases, and should remain at the bottom, IMO).

tprouvot and others added 2 commits April 14, 2022 15:15
Co-authored-by: Sebastien <[email protected]>
Co-authored-by: Sebastien <[email protected]>
@scolladon
Copy link
Owner

Thank you Thomas!

In addition to @scolladon comments, I would add 2 things:

  1. Add the new section to the Table of Contents

  2. I would put it before the U_se the module in your own node application_ section (that last section is for very specific use-cases, and should remain at the bottom, IMO).

@tprouvot I can help on the Table of Content generation, we use a tool to do that automatically, give me access to your repo and I'll contribute

@codeclimate
Copy link

codeclimate bot commented Apr 14, 2022

Code Climate has analyzed commit 13480a7 and detected 0 issues on this pull request.

View more on Code Climate.

Copy link
Collaborator

@mehdicherf mehdicherf left a comment

Choose a reason for hiding this comment

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

LGTM.

Thank you Thomas!

Copy link
Owner

@scolladon scolladon left a comment

Choose a reason for hiding this comment

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

LGTM

@scolladon scolladon changed the title docs: add conditionnal deployment use case to readme docs: add conditional deployment use case to readme Apr 14, 2022
@scolladon scolladon merged commit 7432f55 into scolladon:main Apr 14, 2022
@tprouvot
Copy link
Contributor Author

tprouvot commented Apr 15, 2022

Thank you Thomas!
In addition to @scolladon comments, I would add 2 things:

  1. Add the new section to the Table of Contents
  2. I would put it before the U_se the module in your own node application_ section (that last section is for very specific use-cases, and should remain at the bottom, IMO).

@tprouvot I can help on the Table of Content generation, we use a tool to do that automatically, give me access to your repo and I'll contribute

Hi @scolladon I should have accepted your proposition because I tested the table of content on the new entry and it doesn't redirect to the section... I found the issue on the link and fixed it on my repo. Would you like me to create another PR ?

tprouvot@80f980f

@scolladon
Copy link
Owner

@tprouvot that's a good idea, yes please create another PR (fix type)

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.

Add warning when sgd generates empty package.xml
3 participants