Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Install GraalVM Components in Temp Directory #1660

Merged
merged 1 commit into from
Apr 9, 2021
Merged

Conversation

4e6
Copy link
Contributor

@4e6 4e6 commented Apr 8, 2021

Pull Request Description

Install the required GraalVM components before moving the directory.

It only changes the order of actions. Removing the temporary directory is cheaper and safer than removing the final directory.

Important Notes

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the Scala, Java, and Rust style guides.
  • All documentation and configuration conforms to the markdown and YAML style guides.
  • All code has been tested where possible.

@4e6 4e6 self-assigned this Apr 8, 2021
Copy link
Member

@radeusgd radeusgd left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks for changing that.

Before merging it would be good to manually check (unless you have already done so) if the components are available - to make sure that the logic of installing to temp dir and moving does not break them (I don't see how it could break that, but better be 100% sure).
Actually, ideally, there should be a unit test for that but it may not be possible to implement a reasonable test for that (as the tests should not interfere with the runtime environment and installing a full separate GraalVM as part of tests may take too much additional time on the CI).

@4e6 4e6 merged commit 0aa5b56 into main Apr 9, 2021
@4e6 4e6 deleted the wip/db/install-before-move branch April 9, 2021 09:10
iamrecursion pushed a commit that referenced this pull request Apr 28, 2021
Install the required GraalVM components before 
moving the directory.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants