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

Add a toolchain only - control update of settings.xml, toolchains.xml, JAVA_HOME and PATH more granular #553

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

mhoffrog
Copy link

@mhoffrog mhoffrog commented Nov 7, 2023

09-Nov-2023: Amended commit comments only.

Description:
The following input items have been added (copied from README.md update):

  • update-env-javahome: By default action updates env.JAVA_HOME with the path of java installed. In order to skip update of JAVA_HOME, set this to false. The creation of the env variable JAVA_HOME_{{ MAJOR_VERSION }}_{{ ARCHITECTURE }} is NOT affected by this item. That will be created for any setup.

  • update-env-path: By default action adds <java_install_dir>/bin to env.PATH. In order to skip this, set this to false.

  • update-toolchains-only: If set to true, then overwrite-settings, update-env-javahome and update-env-path are propagated to false if the specific one is not explicitly configured to true. Only a toolchain entry will be added to toolchains.xml. Default is false.

Summary on a few code improvements (Inserted by 09-Nov-2023):
By that implementation I did a few code improvements as follows:

  • setup-java.ts:
    • renamed interface installerInputsOptions -> IInstallerInputsOption and moved the definition to the top of the source file to levarage readability
  • input values for overwrite-settings - plus the ones I did add - are read now once only in a single code place (within setup-java.ts only) to a constant and pass their effective values either by method parameters or by object fields to other places during the runtime lifecycle
  • variable naming within toolchains.ts
  • minor improvements of wording on log messages regarding toolchains.xml updates.

Further particular details on the code changes can be taken from my commit comment b0dadd2.

Related issue:
#552

Check list:

  • Mark if documentation changes are required.
    💡 Documentation has been updated by this PR
  • Mark if tests were added or updated to cover the changes.

@mhoffrog mhoffrog requested a review from a team as a code owner November 7, 2023 14:46
@mhoffrog mhoffrog changed the title Add a toolchain only - control update of settings.xml, JAVA_HOME and PATH more granular Add a toolchain only - control update of settings.xml, toolchains.xml, JAVA_HOME and PATH more granular Nov 8, 2023
@mhoffrog mhoffrog force-pushed the mhoffrog/552_allow_to_add_a_toolchain_only branch 2 times, most recently from 1c8123d to e7b1371 Compare November 8, 2023 22:39
@mhoffrog
Copy link
Author

@IvanZosimov is there anything I can do in addition to get this PR merged?

@mhoffrog mhoffrog force-pushed the mhoffrog/552_allow_to_add_a_toolchain_only branch 2 times, most recently from 3f983c4 to 230bb6e Compare January 31, 2024 18:24
@mhoffrog
Copy link
Author

@IvanZosimov PR branch is re-based to current main and dist scripts did pass Check dist action.

@mhoffrog
Copy link
Author

@IvanZosimov PR branch is re-based to current main and dist scripts did pass Check dist action.

@mhoffrog mhoffrog force-pushed the mhoffrog/552_allow_to_add_a_toolchain_only branch from b8f4969 to 993bbb6 Compare April 7, 2024 18:02
@mhoffrog
Copy link
Author

mhoffrog commented Apr 7, 2024

@mahabaleshwars @IvanZosimov FYI - PR branch is re-based to current main and dist scripts did pass action Check dist

@mhoffrog mhoffrog force-pushed the mhoffrog/552_allow_to_add_a_toolchain_only branch from 4f3734a to 2f3fa47 Compare July 19, 2024 10:45
@mhoffrog
Copy link
Author

@mahabaleshwars @IvanZosimov FYI - PR branch is re-based to current main and dist scripts did pass action Check dist

@mhoffrog mhoffrog force-pushed the mhoffrog/552_allow_to_add_a_toolchain_only branch from 2f3fa47 to 8bb7cab Compare August 23, 2024 21:51
@mhoffrog
Copy link
Author

@mahabaleshwars @IvanZosimov FYI - PR branch is re-based to current main and dist scripts did pass action Check dist

@mhoffrog mhoffrog force-pushed the mhoffrog/552_allow_to_add_a_toolchain_only branch from 8bb7cab to 7425697 Compare December 13, 2024 23:51
…nv-path'

Changes in detail:
------------------
- action.yml:
  - add inputs:
    - update-toolchains-only
    - update-env-javahome
    - add-to-env-path
  - update description for input "overwrite-settings"
  - remove default value of input "overwrite-settings",
    since the default is now propagated from input 'update-toolchains-only'

- base-models.ts:
  - extend interface JavaInstallerOptions:
    - add fields:
      - updateEnvJavaHome: boolean;
      - addToEnvPath: boolean;

- constants.ts:
  - add constant INPUT_UPDATE_TOOLCHAINS_ONLY
    = 'update-toolchains-only'

- auth.ts:
  - function configureAuthentication():
    - add parameter:
      - overwriteSettings: boolean
    - remove the now obsolete const overwriteSettings

- toolchains.ts:
  - function configureToolchains(...):
    - add parameter updateToolchains: boolean
    - remove the now obsolete const overwriteSettings
  - improve variable naming:
    - rename any occurrence of 'overwriteSettings'
        by 'updateToolchains'
    - add field updateToolchains: boolean to the parameter object
  - function writeToolchainsFileToDisk(...):
    - improve variable naming:
      - rename variable 'settingsExists'
          by 'toolchainsExists'
    - update wording of info logs to be more applicable

- setup-java.ts:
  - interface installerInputsOptions:
    - rename to IInstallerInputsOption to meet common coding convention
    - add fields:
      - updateToolchainsOnly: boolean;
      - overwriteSettings: boolean;
      - updateEnvJavaHome: boolean;
      - addToEnvPath: boolean;
  - function run():
    - add const:
      - const updateToolchainsOnly:
        - get as boolean from input 'update-toolchains-only', default: false
      - const overwriteSettings:
        - get as boolean from input 'overwrite-settings', default: !updateToolchainsOnly
      - const updateEnvJavaHome:
        - get as boolean input 'update-env-javahome', default: !updateToolchainsOnly
      - const addToEnvPath:
        - get as boolean input 'add-to-env-path', default: !updateToolchainsOnly
   - extend const installerInputsOptions to match with IInstallerInputsOption:
      - add field updateToolchainsOnly
      - add field overwriteSettings
      - add field updateEnvJavaHome
      - add field addToEnvPath
    - update call of auth.configureAuthentication()
        to auth.configureAuthentication(overwriteSettings)
  - function installVersion(...):
    - add const and init from parameter options:
      - updateToolchainsOnly, overwriteSettings,
        updateEnvJavaHome, addToEnvPath
    - init the additional fields of installerInputsOptions accordingly
    - call toolchains.configureToolchains(...):
      - with parameter updateToolchains= overwriteSettings || updateToolchainsOnly

- base-installer.ts:
  - add constants to import from constants:
    - INPUT_UPDATE_JAVA_HOME
    - INPUT_ADD_TO_PATH
  - add fields:
    - protected updateEnvJavaHome: boolean;
    - protected addToEnvPath: boolean;
  - ctor:
    - init these fields from JavaInstallerOptions accoprdingly
  - function setJavaDefault(...):
    - if updateEnvJavaHome is false:
      - SKIP updating env.JAVA_HOME
      - log info:
        `Skip updating env.JAVA_HOME according to ${INPUT_UPDATE_JAVA_HOME}`
    - if addToEnvPath is false:
      - SKIP adding toolchain path to env.PATH
      - log info:
        `Skip adding to env.PATH according to ${INPUT_ADD_TO_PATH}`
- missed in initial commit
- took re-built dist from GH actions
@mhoffrog mhoffrog force-pushed the mhoffrog/552_allow_to_add_a_toolchain_only branch from 7425697 to cb67ffd Compare December 15, 2024 17:02
@mhoffrog
Copy link
Author

@mahabaleshwars @aparnajyothi-y @HarithaVattikuti FYI - PR branch is re-based to current main and dist scripts did pass action Check dist

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant