layer5io / meshery-smp-action

GitHub Action for pipelining microservices and Kubernetes performance testing with Meshery
https://layer5.io/projects/nighthawk
Apache License 2.0
27 stars 21 forks source link

Modify bash script and action variables #61

Closed alphaX86 closed 1 year ago

alphaX86 commented 1 year ago

Signed-off-by: Aadhitya A aadhitya864@gmail.com

Description

This PR fixes #60 (auth issues alone)

Notes for Reviewers

Signed commits

alphaX86 commented 1 year ago

Ok... I've changed some args in the script, seems now it works... at least for install section image

alphaX86 commented 1 year ago

Also, I've seen that app onboard now asks -s flag for source-type (in Linkerd script)... Is this due to recent change? image

// @theBeginner86

theBeginner86 commented 1 year ago

Also, I've seen that app onboard now asks -s flag for source-type (in Linkerd script)... Is this due to recent change? image

// @theBeginner86

Yeah, mesheryctl was recently updated in response to the effort for adding support for different source types for Applications on Meshery (PR). Since that Linkerd script uses mesheryctl so that is showing up.

gyohuangxin commented 1 year ago

@alphaX86 Is it ok for review?

alphaX86 commented 1 year ago

Yes @gyohuangxin ready for review.

// @hershd23

hershd23 commented 1 year ago

Also got to learn something cool. The entire file meshery.sh file seems to be in the git diff for a 1 line change. This is due to line endings. CRLF vs LF.

A great thread for anyone who wants to know more :- https://stackoverflow.com/questions/37344280/git-diff-is-showing-full-file-has-changed-for-a-single-line-change-but-only-for

In the same vein @leecalcote do we enforce a certain type of line endings? Have seen some orgs being particular about it

leecalcote commented 1 year ago

I'm adding MeshMate @Nikhil-Ladha and maintainer @hexxdump, who might speak to how we do linting and code conventions differently depending about the project.

Nikhil-Ladha commented 1 year ago

Also got to learn something cool. The entire file meshery.sh file seems to be in the git diff for a 1 line change. This is due to line endings. CRLF vs LF.

A great thread for anyone who wants to know more :- https://stackoverflow.com/questions/37344280/git-diff-is-showing-full-file-has-changed-for-a-single-line-change-but-only-for

In the same vein @leecalcote do we enforce a certain type of line endings? Have seen some orgs being particular about it

Hmm....what type did you change to in this PR? We should be following the LF format everywhere and git provides a way to do so by using .gitattributes file. Do check once, to enforce the LF line ending for all files. Like we do for https://github.com/layer5io/layer5 repo.

alphaX86 commented 1 year ago

Also got to learn something cool. The entire file meshery.sh file seems to be in the git diff for a 1 line change. This is due to line endings. CRLF vs LF.

A great thread for anyone who wants to know more :- https://stackoverflow.com/questions/37344280/git-diff-is-showing-full-file-has-changed-for-a-single-line-change-but-only-for

In the same vein @leecalcote do we enforce a certain type of line endings? Have seen some orgs being particular about it

Hmm....what type did you change to in this PR? We should be following the LF format everywhere and git provides a way to do so by using .gitattributes file. Do check once, to enforce the LF line ending for all files. Like we do for https://github.com/layer5io/layer5 repo.

So in short, you want me to revert the whole change in meshery.sh and then do the intended change I wanted to do.... Ok got it

alphaX86 commented 1 year ago

Wait... reverting changes now...

alphaX86 commented 1 year ago

Phew... I found a way to change line endings in Windows... almost it cost me up to change endings of whole repo... I reverted it so no issues. But it cost me a DCO check fail now... even though I sign-off the commit

alphaX86 commented 1 year ago

Umm... @leecalcote are we good to go?

gyohuangxin commented 1 year ago

@alphaX86 Can you resolve the DCO issue?

alphaX86 commented 1 year ago

If I resolve dco issue, it'd result in opening a new PR. Is it OK

gyohuangxin commented 1 year ago

@alphaX86 OK, I merged it. Let's see if it works.

alphaX86 commented 1 year ago

@alphaX86 OK, I merged it. Let's see if it works.

Let's hope ✌️

gyohuangxin commented 1 year ago

@alphaX86 Some performance tests still failed, https://github.com/layer5io/meshery-smp-action/runs/8062696783?check_suite_focus=true. Can you check if your changes worked?