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

implement alternative to allow pull with github token #278

Closed
wants to merge 14 commits into from

Conversation

mad01
Copy link
Contributor

@mad01 mad01 commented Jul 13, 2021

Changes

implementation allowing you to pass github token as suggested in #161 to handle windows/linux/macos and github action envs.

      - name: preload-unity-project
        uses: mad01/unity-builder@main
        env:
          UNITY_EMAIL: ${{ secrets.UNITY_EMAIL }}
          UNITY_PASSWORD: ${{ secrets.UNITY_PASSWORD }}
          UNITY_SERIAL: ${{ secrets.UNITY_SERIAL }}
        with:
          targetPlatform: iOS
          unityVersion: 2018.4.26f1
          buildMethod: X
          gitPrivateToken: ${{ secrets.GIT_PRIVATE_TOKEN }}

Checklist

  • Read the contribution guide and accept the code of conduct
  • Readme (updated or not needed)
  • Tests (added, updated or not needed)

@github-actions
Copy link

Cat Gif

@webbertakken
Copy link
Member

webbertakken commented Jul 13, 2021

Interesting approach! And thank you for your PR.

Please allow us some time to discuss if we want to support this kind of behaviour and whether it would be in this shape.
Especially also whether it is semantically correct or not for the GitHub token to be used as the token for your projects dependency resolution.

In the meantime, have you considered this approach? #256
Could you perhaps also elaborate a bit more on your use case for this?
That way you'd help us understand for which use-cases we'd be supporting this addition.

@mad01
Copy link
Contributor Author

mad01 commented Jul 13, 2021

Sure. i just needed something that worked better in our setup where we have local settings for the packages where they are accessed using https while in the CI setup we needed to use tokens

@mad01
Copy link
Contributor Author

mad01 commented Jul 13, 2021

@webbertakken i did consider #256 but did not entirely get that to work so i spent some time to try to get the token approach to work since that would better support when the manifest.json have none ssh based urls which we have.

@russovincenzo
Copy link

@mad01 I try to use your action but I got this error while the container try to pull repo:

com.art.gradientspacemodule: Cannot checkout repository [https://github.com/<hidden>/GradientSpaceModule.git]:
      Error when executing git command. Downloading GradientSpaceModule~/Assets/StreamingAssets/DeleteFromReleaseBuild/Meshes/TestMesh/BowtieConnectedTriangles.obj (279 B)
      Error downloading object: GradientSpaceModule~/Assets/StreamingAssets/DeleteFromReleaseBuild/Meshes/TestMesh/BowtieConnectedTriangles.obj (dbe3949): Smudge error: Error downloading GradientSpaceModule~/Assets/StreamingAssets/DeleteFromReleaseBuild/Meshes/TestMesh/BowtieConnectedTriangles.obj (dbe39496dec7a4a405d4d5b77b637e473c41b092a4db19aadecbf567bf6b433e): batch response: Bad credentials

      Errors logged to /github/workspace/Library/PackageCache/.tmp129BA7LkSpHqv0j/clone/.git/lfs/objects/logs/20210906T143617.094425013.log
      Use `git lfs logs last` to view the log.
      error: external filter 'git-lfs filter-process' failed
      fatal: GradientSpaceModule~/Assets/StreamingAssets/DeleteFromReleaseBuild/Meshes/TestMesh/BowtieConnectedTriangles.obj: smudge filter lfs failed

Any Idea?

I try to pull the repo with personal access token on my local machine without any issue

@mad01
Copy link
Contributor Author

mad01 commented Sep 9, 2021

I have no idea. i am using it atm and it works to build. here is a example of how it looks for me. A note is that i have only tested this change with unity 2018. We also have all of the internal packages in Packages/manifest.json using https

name: Unity
on: [push]
jobs:

  build-unity:
    runs-on: [self-hosted, Linux, X64, studio]
    strategy:
      max-parallel: 1
    steps:
      # Checkout
      - name: Check repository
        uses: actions/checkout@v2
        with:
          lfs: false
          token: ${{ secrets.PRIVATE_TOKEN }}
          submodules: recursive

      - name: build-unity-project
        uses: mad01/unity-builder@7f1bc277f7beae52cec88a9cc191a2461a1339aa
        env:
          UNITY_LICENSE: ${{ secrets.UNITY_LICENSE }}
        with:
          targetPlatform: iOS
          unityVersion: 2018.4.26f1
          buildMethod: "***.Build.Core.Editor.BatchInterface.PreloadBuildAndExport"
          gitPrivateToken: ${{ secrets.GIT_PRIVATE_TOKEN }}
          allowDirtyBuild: true
          customParameters: |
            -platform iOS
            -defineSymbols 'BUILD_VERBOSE
            -buildNumberOffset 0
            -bundleId com.***.***
            -configurationId com.***.***
            -username='${{ secrets.USERNAME }}' -password='${{ secrets.PASSWORD }}'

else
echo "GIT_PRIVATE_TOKEN is set configuring git credentials"

git config --global credential.helper store
Copy link
Member

Choose a reason for hiding this comment

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

small indentation differences here, not a big problem tho ;)

@GabLeRoux
Copy link
Member

GabLeRoux commented Oct 22, 2021

I don't have too much context here, but this change looks good to me. We'll have to update branch and regenerate dist folder, but overall, if it solves the issue at least for git packages using https (for now), it sounds better than before.

Merging this would also require some documentation update, but I think it's a good step forward 👍

Copy link
Member

@davidmfinol davidmfinol left a comment

Choose a reason for hiding this comment

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

I agree with everything @GabLeRoux said

@webbertakken
Copy link
Member

Agreed then. @mad01 If you could rebase this on our main branch and regenerate the dist files we can run the checks and merge it.

@rYuuk
Copy link

rYuuk commented Oct 28, 2021

Will there be something similar for test runner?

@webbertakken
Copy link
Member

Will there be something similar for test runner?

@rYuuk as we've agreed to move forward with this, you're welcome to port the changes to test-runner as well. The code of test-runner looks very similar or the same in most places.

@davidmfinol
Copy link
Member

@mad01 Can you pull from the main branch and rebuild?
We've already merged the corresponding change into the test runner, and I think it would be good to have this change for builder.

@codecov-commenter
Copy link

Codecov Report

Merging #278 (b8c008b) into main (3636446) will increase coverage by 0.11%.
The diff coverage is 100.00%.

❗ Current head b8c008b differs from pull request most recent head 65ad600. Consider uploading reports for the commit 65ad600 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #278      +/-   ##
==========================================
+ Coverage   56.10%   56.21%   +0.11%     
==========================================
  Files          23       23              
  Lines         770      772       +2     
  Branches      154      155       +1     
==========================================
+ Hits          432      434       +2     
  Misses        337      337              
  Partials        1        1              
Impacted Files Coverage Δ
src/model/build-parameters.ts 100.00% <ø> (ø)
src/model/docker.ts 17.64% <ø> (ø)
src/model/input.ts 100.00% <100.00%> (ø)
src/model/versioning.ts 93.96% <100.00%> (ø)

@davidmfinol
Copy link
Member

I went ahead and merged with #296

# 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.

7 participants