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

fix: update DOCKER CMD on docs/Makefile #1745

Merged
merged 17 commits into from
Jul 27, 2023
Merged

fix: update DOCKER CMD on docs/Makefile #1745

merged 17 commits into from
Jul 27, 2023

Conversation

viktoriussuwandi
Copy link
Contributor

@viktoriussuwandi viktoriussuwandi commented Jul 20, 2023

Fixes #1744

Hi agardnerIT,
just trying to fix issue on file lifecycle-toolkit/docs/Makefile, please check it out

Signed-off-by: Viktorius Suwandi <viktorius.tori@gmail.com>
@viktoriussuwandi viktoriussuwandi requested review from a team as code owners July 20, 2023 02:46
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Jul 20, 2023
@netlify
Copy link

netlify bot commented Jul 20, 2023

Deploy Preview for keptn-lifecycle-toolkit ready!

Name Link
🔨 Latest commit 31877ea
🔍 Latest deploy log https://app.netlify.com/sites/keptn-lifecycle-toolkit/deploys/64c23a5f40917c0008e51b18
😎 Deploy Preview https://deploy-preview-1745--keptn-lifecycle-toolkit.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@agardnerIT agardnerIT changed the title Update DOCKER CMD on docs/Makefile fix: update DOCKER CMD on docs/Makefile Jul 20, 2023
@agardnerIT
Copy link
Contributor

LGTM! Thanks! Closes #1744

@agardnerIT
Copy link
Contributor

Unfortunately it seems I was wrong with my suggested fix. The -it flag works perfectly for us humans (as we do have an interactive terminal) but GitHub Actions apparently doesn't like it 😞

https://github.com/keptn/lifecycle-toolkit/actions/runs/5606326111/jobs/10257542019?pr=1745

@viktoriussuwandi
Copy link
Contributor Author

Unfortunately it seems I was wrong with my suggested fix. The -it flag works perfectly for us humans (as we do have an interactive terminal) but GitHub Actions apparently doesn't like it 😞

https://github.com/keptn/lifecycle-toolkit/actions/runs/5606326111/jobs/10257542019?pr=1745

Hi agardnerIT,
Would you please let me know anything else needed to do to merge this PR ?
Or should it close without merge ?

@agardnerIT
Copy link
Contributor

Due to the issue identified above and the fact that making the change breaks other compatibility (ie. when make server runs in a PR). I think the best way to achieve this would be to add an entirely new task in the Makefile. You can copy and paste the make server command and for this new one, just add the -it flag.

In other words, make server remains as it always has, but this PR introduces a new task which is identical to make server except it includes the -I flag. The intention of this new target would be for contributors (eg. humans) to run this task. The CI processes will continue to run make server as they always have.

When this new task is introduced, the docs/README.md should be updated to update the command.

make server

Perhaps also, add a note to docs/README.md explaining the different between make server and the new command.

Sorry for the extra work, but I just don't see another way to achieve this without breaking other things.

Signed-off-by: Viktorius Suwandi <viktorius.tori@gmail.com>
Signed-off-by: Viktorius Suwandi <viktorius.tori@gmail.com>
@viktoriussuwandi
Copy link
Contributor Author

Thanks for your explanation @agardnerIT,

Actually I don't know how to apply your idea, but this is what I have tried to do :

  1. Restore docs/Makefile as before with just -i flag on line 7

  2. Create new file docs/Makeserver with exactly same code as docs/Makefile, with -it flag on line 7

Please let me know anything need else to do or improve. Appreciate your guidance !

@aepfli
Copy link
Member

aepfli commented Jul 24, 2023

i would not suggest to duplicate this file. It creates confusion, and does not make it logical on why to use it or not.

eg. this can be determined within the makefile (eg. https://stackoverflow.com/a/4251643/3708208) and we can set a variable based on this definition. Such an approach would make it way clearer to the users, und why there is a difference, and in the end, the end user does not want to ensure a proper file or not.

@mowies
Copy link
Member

mowies commented Jul 25, 2023

what @aepfli said :D

Copy link
Member

@mowies mowies left a comment

Choose a reason for hiding this comment

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

@viktoriussuwandi pls check the comment here: #1745 (comment)

Signed-off-by: Viktorius Suwandi <viktorius.tori@gmail.com>
Signed-off-by: Viktorius Suwandi <viktorius.tori@gmail.com>
@viktoriussuwandi
Copy link
Contributor Author

Hi @mowies
I have tried to update as mentioned by the reference, hopefully it will run as expected.

@aepfli Thanks for your suggestion! Also need your feedback on this

docs/Makefile Outdated Show resolved Hide resolved
Co-authored-by: Moritz Wiesinger <moritz.wiesinger@dynatrace.com>
Signed-off-by: Viktorius Suwandi <viktorius.tori@gmail.com>
Copy link
Member

@mowies mowies left a comment

Choose a reason for hiding this comment

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

one small change, other than that, LGTM

docs/Makefile Outdated Show resolved Hide resolved
viktoriussuwandi and others added 2 commits July 27, 2023 16:35
Co-authored-by: Moritz Wiesinger <moritz.wiesinger@dynatrace.com>
Signed-off-by: Viktorius Suwandi <viktorius.tori@gmail.com>
@viktoriussuwandi
Copy link
Contributor Author

no problem @mowies, Thanks for your suggestion!

Copy link
Member

@mowies mowies left a comment

Choose a reason for hiding this comment

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

LGTM

@mowies mowies merged commit a9ac9f6 into keptn:main Jul 27, 2023
12 checks passed
@sonarcloud
Copy link

sonarcloud bot commented Jul 27, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@agardnerIT
Copy link
Contributor

That's an awesome solution. Definitely learned something here!

@viktoriussuwandi
Copy link
Contributor Author

Thanks for this opportunity @ALL

@viktoriussuwandi viktoriussuwandi deleted the issue#1744 branch July 27, 2023 13:29
@aepfli
Copy link
Member

aepfli commented Jul 28, 2023

Nice Solution! well done!

prakrit55 pushed a commit to prakrit55/lifecycle-toolkit that referenced this pull request Aug 3, 2023
Co-authored-by: Moritz Wiesinger <moritz.wiesinger@dynatrace.com>
Signed-off-by: Griffin <prakritimandal611@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Docs] make server doesn't kill container
5 participants