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

Modify bash script and action variables #61

Merged
merged 10 commits into from
Aug 29, 2022

Conversation

alphaX86
Copy link
Member

@alphaX86 alphaX86 commented Aug 11, 2022

Signed-off-by: Aadhitya A [email protected]

Description

This PR fixes #60 (auth issues alone)

Notes for Reviewers

Signed commits

  • Yes, I signed my commits.

Signed-off-by: Aadhitya A <[email protected]>
Signed-off-by: Aadhitya A <[email protected]>
@alphaX86
Copy link
Member Author

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

@alphaX86
Copy link
Member Author

alphaX86 commented Aug 11, 2022

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
Copy link
Member

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.

Copy link
Member

@gyohuangxin gyohuangxin left a comment

Choose a reason for hiding this comment

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

Thanks for fixing it.

Signed-off-by: Aadhitya A <[email protected]>
Signed-off-by: Aadhitya A <[email protected]>
@gyohuangxin
Copy link
Member

@alphaX86 Is it ok for review?

@alphaX86
Copy link
Member Author

Yes @gyohuangxin ready for review.

// @hershd23

Copy link
Member

@gyohuangxin gyohuangxin left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@hershd23 hershd23 left a comment

Choose a reason for hiding this comment

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

Just a couple of comments. Otherwise LGTM

@hershd23
Copy link
Contributor

hershd23 commented Aug 12, 2022

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
Copy link
Member

leecalcote commented Aug 12, 2022

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
Copy link

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
Copy link
Member Author

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

Signed-off-by: Aadhitya A <[email protected]>
@alphaX86
Copy link
Member Author

Wait... reverting changes now...

This reverts commit b733aaa.

Signed-off-by: Aadhitya A <[email protected]>
Signed-off-by: Aadhitya A <[email protected]>
@alphaX86
Copy link
Member Author

alphaX86 commented Aug 13, 2022

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 alphaX86 requested a review from hershd23 August 16, 2022 04:14
Copy link
Contributor

@hershd23 hershd23 left a comment

Choose a reason for hiding this comment

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

LGTM

@alphaX86
Copy link
Member Author

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

Copy link
Member

@gyohuangxin gyohuangxin left a comment

Choose a reason for hiding this comment

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

LGTM

@alphaX86 alphaX86 requested review from leecalcote and removed request for leecalcote August 24, 2022 13:06
@gyohuangxin
Copy link
Member

@alphaX86 Can you resolve the DCO issue?

@alphaX86
Copy link
Member Author

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

@gyohuangxin gyohuangxin merged commit bb2f67c into layer5io:master Aug 29, 2022
@gyohuangxin
Copy link
Member

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

@alphaX86
Copy link
Member Author

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

Let's hope ✌️

@gyohuangxin
Copy link
Member

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Stability issues wrt Scheduled Benchmark Tests on Self Hosted Runner
6 participants