osscameroon / js-generator

Generates JavaScript from HTML
https://osscameroon.github.io/js-generator/
MIT License
19 stars 14 forks source link

Run mvn site -B -e -X --projects 'jsgenerator-core' only when the code is merged on main. #255

Closed FanJups closed 1 year ago

FanJups commented 1 year ago

Currently, every PR triggers the deployment of GitHub Pages. That's not a good behaviour.

Tip: create another yaml file javadoc.yml with this config and update .github/workflows/maven.yml accordingly

name: Maven Site

on:
  push:
    branches: [ main ]

jobs:
  build:

    runs-on: ubuntu-latest

    steps:
    - uses: actions/checkout@v3
    - name: Set up JDK 17
      uses: actions/setup-java@v3
      with:
        distribution: 'temurin'
        java-version: 17
        # Given the fact that this is a multimodule project, build process will take long time so we activate caching
        # To know more: https://maven.apache.org/extensions/maven-build-cache-extension/cache.html
        cache: 'maven'
    - name: Build Javadoc Site with Maven
    #To see the full stack trace of the errors, re-run Maven with the -e switch.
    #Re-run Maven using the -X switch to enable full debug logging.
    # -B,--batch-mode Run in non-interactive (batch) mode (disables output color)
    # To learn more about options: https://maven.apache.org/ref/3.6.3/maven-embedder/cli.html
      run: |
        mvn site -B -e -X --projects 'jsgenerator-core'
      env:
        MAVEN_SITE_GITHUB_OAUTH_TOKEN: ${{ secrets.MAVEN_SITE_GITHUB_OAUTH_TOKEN }}
FanJups commented 1 year ago

@shrutikumar15, maybe this will interest you.

shrutikumar15 commented 1 year ago

Do you want to remove this part of the maven.yml file

pull_request_target:
    types: [ opened, edited, synchronize, ready_for_review, review_requested, review_request_removed ]
    branches: [ main ] 

I did not understand why we need two files javadoc.yml and maven.yml

FanJups commented 1 year ago

Do you want to remove this part of the maven.yml file

pull_request_target:
    types: [ opened, edited, synchronize, ready_for_review, review_requested, review_request_removed ]
    branches: [ main ] 

I did not understand why we need two files javadoc.yml and maven.yml

I am trying to run "clean package" ( or any goal executing test ) for PR and branch main but "site" is only for main since PR is not yet ready.

I think the easiest way to achieve that is using 2 files but maybe I'm wrong. What do you think ?

FanJups commented 1 year ago

Do you want to remove this part of the maven.yml file

pull_request_target:
    types: [ opened, edited, synchronize, ready_for_review, review_requested, review_request_removed ]
    branches: [ main ] 

I did not understand why we need two files javadoc.yml and maven.yml

No need to remove this part inside maven.yml because we want to run clean package for PR and main

FanJups commented 1 year ago

Currently, every PR triggers the deployment of GitHub Pages. That's not a good behaviour.

Tip: create another yaml file javadoc.yml with this config and update .github/workflows/maven.yml accordingly

name: Maven Site

on:
  push:
    branches: [ main ]

jobs:
  build:

    runs-on: ubuntu-latest

    steps:
    - uses: actions/checkout@v3
    - name: Set up JDK 17
      uses: actions/setup-java@v3
      with:
        distribution: 'temurin'
        java-version: 17
        # Given the fact that this is a multimodule project, build process will take long time so we activate caching
        # To know more: https://maven.apache.org/extensions/maven-build-cache-extension/cache.html
        cache: 'maven'
    - name: Build Javadoc Site with Maven
    #To see the full stack trace of the errors, re-run Maven with the -e switch.
    #Re-run Maven using the -X switch to enable full debug logging.
    # -B,--batch-mode Run in non-interactive (batch) mode (disables output color)
    # To learn more about options: https://maven.apache.org/ref/3.6.3/maven-embedder/cli.html
      run: |
        mvn site -B -e -X --projects 'jsgenerator-core'
      env:
        MAVEN_SITE_GITHUB_OAUTH_TOKEN: ${{ secrets.MAVEN_SITE_GITHUB_OAUTH_TOKEN }}

We should only remove mvn site and env from maven.yml

FanJups commented 1 year ago

maven.yml will look like this

# This workflow will build a Java project with Maven
# For more information see: https://help.github.com/actions/language-and-framework-guides/building-and-testing-java-with-maven

name: Java CI with Maven

on:
  push:
    branches: [ main ]
  pull_request_target:
    types: [ opened, edited, synchronize, ready_for_review, review_requested, review_request_removed ]
    branches: [ main ]

jobs:
  build:

    runs-on: ubuntu-latest

    steps:
    - uses: actions/checkout@v3
    - name: Set up JDK 17
      uses: actions/setup-java@v3
      with:
        distribution: 'temurin'
        java-version: 17
        # Given the fact that this is a multimodule project, build process will take long time so we activate caching
        # To know more: https://maven.apache.org/extensions/maven-build-cache-extension/cache.html
        cache: 'maven'
    - name: Build with Maven
    #To see the full stack trace of the errors, re-run Maven with the -e switch.
    #Re-run Maven using the -X switch to enable full debug logging.
    # -B,--batch-mode Run in non-interactive (batch) mode (disables output color)
    # To learn more about options: https://maven.apache.org/ref/3.6.3/maven-embedder/cli.html
      run: |
        mvn package -B -e -X
FanJups commented 1 year ago

Do you want to remove this part of the maven.yml file

pull_request_target:
    types: [ opened, edited, synchronize, ready_for_review, review_requested, review_request_removed ]
    branches: [ main ] 

I did not understand why we need two files javadoc.yml and maven.yml

To sum up, javadoc.yml is only for main and maven.yml is for PR and main.

When a PR is created, we only test the code. But once it is merged, we test through maven.yml and generate site through javadoc.yml Every change is not made through PR, some safe changes are done directly on main. As an illustration, 2 days ago I updated README on main then test & site were triggered.

FanJups commented 1 year ago

Current Behaviour :

Every PR runs package and site goals. As a result, GitHub Pages are deployed for every PR because of site goal.

Expected behaviour:

Every new PR should only run package goal. Every Merged PR and push should run package and site goals.

FanJups commented 1 year ago

I am also thinking 🤔, maybe test goal should be enough ? package goal seems to do extra work we don't need now. Maybe, it will be useful in the future but now looks like its useless. What do you think about replacing package with test ?

Adding clean will be a plus:

mvn clean site mvn clean test

FanJups commented 1 year ago

@shrutikumar15 , please do you need more explanation ?

shrutikumar15 commented 1 year ago

@FanJups Sorry I did not see your comments, I have raised a PR - #257