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 scheduled builds #36

Merged
merged 1 commit into from
Feb 16, 2022
Merged

Fix scheduled builds #36

merged 1 commit into from
Feb 16, 2022

Conversation

AugustasV
Copy link
Contributor

@AugustasV AugustasV commented Feb 13, 2022

Description

Notes for Reviewers
Was looking into this issue why builds been failing, find out that Open Service Mesh (OSM) CLI version was changed, also tried to simplify code to see what can I do to make it work.

Unsuccessful run on master here :- https://github.com/layer5io/meshery-smp-action/runs/5204098138?check_suite_focus=true
Successful run on your local fork :- https://github.com/AugustasV/meshery-smp-action/runs/5204058656?check_suite_focus=true

Signed commits

  • Yes, I signed my commits.

@leecalcote
Copy link
Member

// @hershd23

@leecalcote leecalcote added the area/ci Continuous integration | Build and release label Feb 15, 2022
@leecalcote
Copy link
Member

Thank you, @AugustasV! 👍

@gyohuangxin
Copy link
Member

@AugustasV Thank you, and this code looks good to me. But a little suggestion, could you rebase your 5 commits to one commit, because they are doing the same thing, which will help keep commit log clean.

@hershd23
Copy link
Contributor

hershd23 commented Feb 15, 2022

Hi @AugustasV great work and thank you for taking up this problem.

Just a couple of suggestions :-

  1. As @gyohuangxin has said if you could squash these 5 commits into one, that would really help up keeping the log clean. You can follow the instructions given here. https://stackoverflow.com/questions/5189560/squash-my-last-x-commits-together-using-git Both the highest voted and the accepted answer should work
  2. Do link the successful workflow run post the change you have made.

Edit :- @AugustasV I have done point 2 for you here, do confirm it for me though
Unsuccessful run on master here :- https://github.com/layer5io/meshery-smp-action/runs/5204098138?check_suite_focus=true
Successful run on your local fork :- https://github.com/AugustasV/meshery-smp-action/runs/5204058656?check_suite_focus=true

I believe after this your changes would be ready to merge

Signed-off-by: AugustasV <[email protected]>
@AugustasV AugustasV requested a review from pottekkat February 15, 2022 18:25
@AugustasV
Copy link
Contributor Author

Thank you for suggestions and tips, squashed.

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.

Approved

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.

Copy link
Contributor

@pottekkat pottekkat left a comment

Choose a reason for hiding this comment

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

The PR was linked to the wrong issue. Removed that from the PR description.

LGTM.

@pottekkat pottekkat merged commit 1b0d804 into layer5io:master Feb 16, 2022
@AugustasV AugustasV deleted the fix_scheduled_builds branch February 19, 2022 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ci Continuous integration | Build and release
Development

Successfully merging this pull request may close these issues.

5 participants