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

Reusable workflow updates #94

Merged
merged 25 commits into from
Nov 8, 2024

Conversation

timkimadobe
Copy link
Contributor

@timkimadobe timkimadobe commented Oct 30, 2024

Questions for reviewers

1. Tag strategy, given that we are hardcoding the tags inside the reusable workflows (in order to be able to nest reusable workflows). Some options:
* Date based tag: gha-ios-2024.10.30 OR gha-2024.10.30 (without platform)
* Semantic version based tag: gha-ios-1.0.0
* Also can be matched to the major version of the extensions it applies to, like AEPTestUtils

Going with platform based major version aligned tag strategy (similar to AEPTestUtils):

  • gha-ios-5.0.0
  • gha-android-3.0.0

2. Should the release actions be allowed to rerun without failure if a GitHub release with the same tag already exists? This could be useful in cases where the action needs to be rerun, such as for an expired CocoaPods token.
* Currently, the action fails if a GitHub release with the given tag already exists, which can happen if the release was created successfully in a previous run but failed afterward due to other issues.

Applying the following logic, where the release and publish pod steps are independent of each other (aside from the publish pod's check of tag existing):

if (create_github_release) {
    // Create a tag and GitHub release; fail if the tag already exists.
} 

if (release_pods) { 
    // Check if the tag exists.
    // Publish pods.
}

Description

Important

This PR changes how tags are used within the reusable workflows. Before, the tags were passed to the workflows and now they are hardcoded to match the tag they will be released with.

This means that the tag maintenance burden is on the aepsdk-commons side and not the caller, and also enables us to more easily use nested reusable workflows.

jobs:
  validate-versions:
    name: Validate Versions
    # Making use of existing reusable workflow for version update/validation
    uses: adobe/aepsdk-commons/.github/workflows/versions.yml@gha-1.0.0 
    with:
      version: ${{ github.event.inputs.tag }}
      branch: ${{ github.ref_name }}
      paths: ${{ inputs.version-validation-paths }}
      dependencies: ${{ inputs.version-validation-dependencies }}
      update: false

  release:
    runs-on: macos-latest
    # Make the rest of the release job dependent on version validation passing, and don't 
    # need to add all the script caching + downloading boilerplate
    needs: validate-versions
    steps:
    - uses: actions/checkout@v4.1.7
    ...

This PR fixes the implementation of the reusable workflows: versions.yml and ios-release.yml, updated input var names in android-maven-release.yml and updates documentation.

ios-release.yml

  1. Fixed incorrect version validation step (thank you for helping catch this @addb) which was relying on caller repo's local Makefile and version.sh. Replaced with usage of Commons version.py script with associated cache + checkout logic.

versions.yml

  1. Fixes logic to only create PRs when in update mode, and removed previous git add exclusion for aepsdk-commons scripts
  2. Simplified update flag logic by using GitHub Actions ternary instead of shell scripting
  3. Reordered inputs to put mandatory paths before dependencies
  4. [Breaking change] Renamed version-verify-paths -> version-validation-paths for clarity
  5. [Breaking change] Renamed version-verify-dependencies -> version-validation-dependencies for clarity

Text/doc related changes:

  1. Renamed "verify" to "validate" for clarity
  2. Updated usages of semantic version to 1.2.3
  3. General description documentation improvements

android-maven-release.yml

  1. [Breaking change] Renamed version-verify-paths -> version-validation-paths for clarity
  2. [Breaking change] Renamed version-verify-dependencies -> version-validation-dependencies for clarity

versions.py

Updated documentation for consistency with changes in versions.yml

workflows.md

Newly created documentation that covers requirements for reusable workflow usage

Example job runs

Using the fixed versions logic in iOS release:

Validate versions run:

Example of error output from version validation fail:

Screenshot 2024-10-30 at 3 56 06 PM

iOS Release with new chained version validation workflow

Android release with new chained version validation workflow

Android update versions example:

Alternative implementations

For the version validation logic used in the iOS and Android release workflows, I tried to simplify the amount of boilerplate steps surrounding cache, checkout, using the script, cleanup by exploring the alternatives:

Using Javascript workflow dispatch

This could work but is more cumbersome because a PAT is required since GITHUB_TOKEN auto-authorization for this kind of dispatch only works for workflow_dispatch and repository_dispatch events (our reusable workflows use workflow_call)

# TEST: using workflow dispatch to programmatically set tag
- uses: actions/github-script@v7
  with:
    github-token: ${{ secrets.GITHUB_TOKEN }}
    debug: true
    script: |
      const workflow = await github.rest.actions.createWorkflowDispatch({
          owner: "adobe",
          repo: "aepsdk-commons",
          workflow_id: "versions.yml",
          ref: "${{ inputs.workflow_tag }}",
          inputs: {
            version: "${{ inputs.tag }}",
            branch: "main",
            paths: "${{ inputs.version-validation-paths }}",
            dependencies: "${{ inputs.version-validation-dependencies }}",
            workflow_tag: "${{ inputs.workflow_tag }}"
          }
      });
      console.log(workflow);

Using composable action

This doesnt work for the same reason that the version.py script is not accessible from the context of the caller repo - if you want to use a composite action defined in aepsdk-commons you need to download the action definition somehow and you're back where you started.

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

…tion

Fix input descriptions
Update logic to use explicit cache save and file removal instead of git add ignore
@timkimadobe timkimadobe requested review from praveek and addb October 30, 2024 01:56

**Example:**
iOS: `"AEPCore 3.1.1, AEPServices 8.9.10@AEPCore.podspec, Edge 3.2.1@Package.swift"`
Android: `"AEPCore 7.8.9, AEPEdgeIdentity 8.9.10@code/gradle.properties"`
Android: `"AEPCore 7.8.9, AEPEdgeIdentity 8.9.10@code/gradle.properties;code/example.kt"`
Copy link

Choose a reason for hiding this comment

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

why are we adding code/example.kt is it also an intended sample? If yes we can call it code/constants.kt for better context. If we want to add this let's add the sample for iOS as well.

Copy link

Choose a reason for hiding this comment

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

we can also give full samples like on L63

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! I updated them to be more realistic examples

path: .github/aepsdk-commons/scripts
key: aepsdk-commons-${{ inputs.workflow_tag }} # Cache key format: <REPO_NAME>-<TAG>

- name: Update or verify versions
run: |
UPDATE_FLAG=""
if [ "${{ inputs.update }}" = "true" ]; then
Copy link

Choose a reason for hiding this comment

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

inputs.update can be treated as boolean here, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! I actually simplified it further by just directly using a GitHub Actions ternary instead of using shell logic - what do you think?

@timkimadobe timkimadobe requested review from addb and cacheung October 30, 2024 23:22
@praveek
Copy link
Contributor

praveek commented Nov 5, 2024

@timkimadobe

1. Tag strategy, given that we are hardcoding the tags inside the reusable workflows (in order to be able to nest reusable workflows). Some options:
    Date based tag: gha-ios-2024.10.30 OR gha-2024.10.30 (without platform)
    Semantic version based tag: gha-ios-1.0.0
        Also can be matched to the major version of the extensions it applies to, like AEPTestUtils
2. Should the release actions be allowed to rerun without failure if a GitHub release with the same tag already exists? This could be useful in cases where the action needs to be rerun, such as for an expired CocoaPods token.
    Currently, the action fails if a GitHub release with the given tag already exists, which can happen if the release was created successfully in a previous run but failed afterward due to other issues.
  1. After giving it a bit more thought, I prefer using tags synced with major versions instead of date. The main reason is that we may end up supporting the current major version alongside the previous one for a period before deprecating it. Keeping separate major versions would make managing both much easier.
  2. I expect the following order of operations.
if (create_github_release) {
    // Create a tag and GitHub release; fail if the tag already exists.
} 

if (release_pods) { 
    // Check if the tag exists.
    // Publish pods.
}

required: false
default: ''
Copy link

Choose a reason for hiding this comment

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

Shouldn't this default be removed as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this default should be ok, since the empty string case is taken as no dependencies being specified? Or were you referring to something else?


# This workflow requires the following GitHub repository settings:
# 1. Navigate to: Settings -> Code and automation -> Actions -> General.
# 2. Under "Workflow permissions," select:
# - "Allow GitHub Actions to create and approve pull requests."

env:
Copy link

Choose a reason for hiding this comment

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

Why are we not adding WORKFLOW_TAG to other workflows like release_ios, android etc..?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is because of where the value is used - for the uses: field (used in ios-release.yml, for example), using dynamic values is not possible (ex: ${{ env.WORKFLOW_TAG }})

However, it does work in this case due to the context it is used in this workflow

@timkimadobe timkimadobe requested a review from addb November 8, 2024 00:18
@timkimadobe timkimadobe merged commit 5b34153 into adobe:main Nov 8, 2024
2 checks passed
@timkimadobe timkimadobe deleted the reusable-workflow-updates branch December 4, 2024 23:17
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