Skip to content

[SHIR] Install Microsoft JDK and then delete msi to decrease image size #17

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

Merged
merged 2 commits into from
Sep 11, 2023

Conversation

missingcharacter
Copy link
Contributor

@missingcharacter missingcharacter commented Aug 8, 2023

Included changes:

  • [SHIR] Install Microsoft JDK and then delete msi to decrease image size
  • [SHIR] Remove SHIR msi to decrease image size

@missingcharacter
Copy link
Contributor Author

@microsoft-github-policy-service agree

@missingcharacter
Copy link
Contributor Author

BTW, I tested the resulting image ghcr.io/missingcharacter/adf-shir:PR-1.6.57b4dde myself and it works, I am able to register a node and able to create parquet files

@missingcharacter missingcharacter force-pushed the MSFT-OpenJDK-and-host-container branch from 57b4dde to da6a65a Compare August 9, 2023 19:21
@jikuja
Copy link

jikuja commented Aug 13, 2023

BTW, I tested the resulting image ghcr.io/missingcharacter/adf-shir:PR-1.6.57b4dde myself and it works, I am able to register a node and able to create parquet files

Which container orchestration did you use?

Copy link

@jikuja jikuja left a comment

Choose a reason for hiding this comment

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

Nice additions and JRE/JDK installation was something to add before running this in any production workload.


Related ticket: https://github.com/MicrosoftDocs/azure-docs/issues/113486

I did not add any comments for https://github.com/Azure/Azure-Data-Factory-Integration-Runtime-in-Windows-Container/pull/17/files#diff-38ab18146fe7ed8ca5f34789d90179e6212e48f778045320c3a129d18a456849R57-R59

Documentation is really bad but from SO questions, some parts of the documentation and error messages we can see that SHIR tries first to use registry keys and than JAVA_HOME to locate java.dll. Nothing suggests that PATH changes are needed for SHIR. If usage of $JavaHome can be removed from build.ps1 it will significantly simplify required logic for build-time java installer selection.

@missingcharacter
Copy link
Contributor Author

Which container orchestration did you use?

@jikuja , I tested on Azure App Service on a Service Plan ith Hyper-V: true

@missingcharacter missingcharacter force-pushed the MSFT-OpenJDK-and-host-container branch from 3c9e384 to 12d49db Compare August 16, 2023 00:31
@missingcharacter missingcharacter force-pushed the MSFT-OpenJDK-and-host-container branch from 12d49db to 0ac4ee4 Compare August 16, 2023 01:21
@byran77
Copy link
Collaborator

byran77 commented Sep 5, 2023

Hi @missingcharacter,
nice points to improve. It would be better to separate the changes into different PRs.

@jikuja
Copy link

jikuja commented Sep 5, 2023

I have been re-iterating this idea on my head...

Would it be possible to do following:

  • create SHIR/scripts/.placeholder
  • move code that installs JVM into contrib/01-install-jvm.ps1
  • modify build.ps1 to execute .ps1 files SHIR/scripts
  • document this

This would make easier to extend capabilities by adding files just before build process.

@byran77
Copy link
Collaborator

byran77 commented Sep 6, 2023

I have been re-iterating this idea on my head...

Would it be possible to do following:

  • create SHIR/scripts/.placeholder
  • move code that installs JVM into contrib/01-install-jvm.ps1
  • modify build.ps1 to execute .ps1 files SHIR/scripts
  • document this

This would make easier to extend capabilities by adding files just before build process.

lgtm
It is reasonable to keep only the basic building logic in build.ps1 for better maintainability.

@missingcharacter
Copy link
Contributor Author

@byran77 I moved GitHub Action to its own PR #19

@missingcharacter missingcharacter force-pushed the MSFT-OpenJDK-and-host-container branch from 8263493 to 7a1ec12 Compare September 6, 2023 23:47
@missingcharacter
Copy link
Contributor Author

@byran77 I've updated this PR too

@byran77
Copy link
Collaborator

byran77 commented Sep 8, 2023

thanks @missingcharacter, it looks good.
One more thing is to add some documents on the building argument INSTALL_JDK

@missingcharacter
Copy link
Contributor Author

@byran77 I added docs

@byran77 byran77 merged commit b4989a0 into Azure:main Sep 11, 2023
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.

3 participants