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

Make OTP installation optional #81

Merged
merged 6 commits into from
Jan 12, 2022
Merged

Make OTP installation optional #81

merged 6 commits into from
Jan 12, 2022

Conversation

michallepicki
Copy link
Contributor

closes #79

@paulo-ferraz-oliveira
Copy link
Collaborator

@michallepicki, thanks for this. Let me check the changes and get back to you.

@paulo-ferraz-oliveira
Copy link
Collaborator

Only a couple minor changes and we're almost there.

@michallepicki
Copy link
Contributor Author

Not sure what this failure is about? https://github.com/erlef/setup-beam/runs/4762594908?check_suite_focus=true

@paulo-ferraz-oliveira
Copy link
Collaborator

Don't worry if the build is only failing for Check if build left artifacts, since this was not introduced by you.

@paulo-ferraz-oliveira
Copy link
Collaborator

Not sure what this failure is about? https://github.com/erlef/setup-beam/runs/4762594908?check_suite_focus=true

It's because the 3rd party licenses we're using were updated remotely but not locally (then the action updates them, checks against what you pushed, and errors out).

@paulo-ferraz-oliveira
Copy link
Collaborator

I approve the current set of changes. Thanks much for your contribution 👍. I'd like to wait for @starbelly to review this, too, and maybe handle #76 before releasing a new minor.

@starbelly
Copy link
Member

@michallepicki You should be able to rebase and we'll get a full passing build.

Copy link
Member

@starbelly starbelly left a comment

Choose a reason for hiding this comment

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

This looks good to me, and I mean that :)

Approving with the caveat that the build is still failing.

Copy link
Member

@starbelly starbelly left a comment

Choose a reason for hiding this comment

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

Almost forgot, we need documentation for this. It should read clearly this option is currently only applicable to gleam.

Copy link
Contributor Author

@michallepicki michallepicki left a comment

Choose a reason for hiding this comment

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

@starbelly Readme and action.yml updated!

@paulo-ferraz-oliveira
Copy link
Collaborator

I left 2 comments and 2 code change suggestions. I'd wondered about the document changes, but had thought making the option visible would only bring more confusion. As is (current pull request), it might not.

@michallepicki
Copy link
Contributor Author

Rebased from main to resolve conflict

Co-authored-by: Paulo F. Oliveira <paulo.ferraz.oliveira@gmail.com>
@paulo-ferraz-oliveira
Copy link
Collaborator

@michallepicki, so it's easier for me to follow up on what was request/done, resolve conversations only after you push the code changes (or even don't resolve them and let the one who opened them do). 👍

@paulo-ferraz-oliveira
Copy link
Collaborator

I'm re-running all actions, after which I think we're good to merge.

@paulo-ferraz-oliveira
Copy link
Collaborator

I believe the incomplete action runs are due to us still using ubuntu-16.04, but that's been handled in a different pull request, soon to be merged to main. I'm squash merging this, and once we have all the open pull requests merged and no more action issues we're good to release a new minor. Thanks much.

@paulo-ferraz-oliveira paulo-ferraz-oliveira requested review from starbelly and removed request for starbelly January 12, 2022 11:59
@paulo-ferraz-oliveira
Copy link
Collaborator

Hm... it seems I can't merge since @starbelly has requested changes and these need to be approved (I even tried to remove him as reviewer to speed this up, but no good). We'll have to wait.

@starbelly
Copy link
Member

approved

@paulo-ferraz-oliveira paulo-ferraz-oliveira merged commit 3179686 into erlef:main Jan 12, 2022
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.

Make OTP installation optional
3 participants