Skip to content
This repository was archived by the owner on Jun 11, 2024. It is now read-only.

Conversation

@asolntsev
Copy link
Contributor

No description provided.

Copy link
Contributor

@ericbeland ericbeland left a comment

Choose a reason for hiding this comment

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

Looks ok except for my hesitation around the windows changes to gradlew. They might also be fine, but before I sign off, I want to understand the rationale.


goto fail

:init
Copy link
Contributor

Choose a reason for hiding this comment

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

Have these changes been tested on Windows? I don't have a Windows machine handy. Can you provide a rationale for the gradlew.bat changes so I can understand better why we should remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi.
This file is automatically updated when you run command

./gradlew wrapper --gradle-version=6.7.1 --distribution-type=bin

It's the official recommended way to upgrade gradle.

No, I personally haven't tried to run this file on Windows, but I don't see any reasons why it should not work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perfect, thanks for the explanation!

@asolntsev
Copy link
Contributor Author

asolntsev commented Dec 3, 2020

@ericbeland I have resolved conflicts and rebased the branch on latest master branch.

@ericbeland
Copy link
Contributor

I'm ready to merge this, but can we pull out the minor bump on awaitility in this PR? We have a PR here #270 generated by dependabot that does the awaitility bump, and that minor bump keeps failing to build (3 times), so I was planning to revisit that later.

@asolntsev
Copy link
Contributor Author

@ericbeland Sorry, I don't understand what you are asking for.

  1. PR Bump awaitility from 4.0.2 to 4.0.3 #270 upgrades org.awaitility:awaitility 4.0.2 -> 4.0.3
  2. But current PR Upgrade dependencies #317 already contains org.awaitility:awaitility:4.0.3. Nothing to pull here.

implementation 'javax.xml.bind:jaxb-api:2.3.1'

implementation 'org.awaitility:awaitility:4.0.2'
implementation 'org.awaitility:awaitility:4.0.3'
Copy link
Contributor

Choose a reason for hiding this comment

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

When dependabot generated a PR for awaitiity 4.0.3, I could not for the life of me get the build tests to pass. I closed it out. I haven't done a dive into why our tests consistently failed, but until I do, I would be nervous to merge a PR with 4.0.3 in it.

@ericbeland ericbeland merged commit 602b0ea into browserup:master Dec 6, 2020
@asolntsev asolntsev deleted the upgrade-dependencies branch December 11, 2021 13:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants