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

feat: add compatibility with tutor nightly #3

Merged
merged 5 commits into from
Sep 18, 2024
Merged

Conversation

dcoa
Copy link
Contributor

@dcoa dcoa commented Sep 13, 2024

This PR creates a new step to run tutor nightly if is configured in the tutor_version matrix

You can check how to use it here eduNEXT/eox-tenant#216 (also the result with <19.0.0 and nightly)

@dcoa dcoa requested a review from a team September 13, 2024 01:16
Copy link
Contributor

@BryanttV BryanttV left a comment

Choose a reason for hiding this comment

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

Thank you! @dcoa, I just have a few comments

action.yml Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
@dcoa dcoa changed the title Dcoa/test int nightly feat: add compatibility with tutor nightly Sep 13, 2024
Copy link

@bra-i-am bra-i-am left a comment

Choose a reason for hiding this comment

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

It looks good to me. Nevertheless, if it can be done the thing of unifying the lines I would look at it better :D

please let me know what you think to send my approval

action.yml Outdated Show resolved Hide resolved
action.yml Outdated
@@ -15,6 +15,7 @@ runs:
using: 'composite'
steps:
- name: Prepare Tutor and Launch
if: ${{ env.INPUT_TUTOR_VERSION != 'nightly' }}

Choose a reason for hiding this comment

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

If it's possible to execute nightly directly with the tutor local launch -I, I think we can name this step like Install Tutor and unify the Tutor installations here

@dcoa
Copy link
Contributor Author

dcoa commented Sep 17, 2024

Note

This PR could be impacted for #4, I'm going to take that changes in mind to update the action to integrate nightly.

@magajh
Copy link
Contributor

magajh commented Sep 17, 2024

Note

This PR could be impacted for #4, I'm going to take that changes in mind to update the action to integrate nightly.

@dcoa Since this PR is much closer to getting merged, I will incorporate the changes from your PR into mine once it's closed to support running the tests in Tutor nightly. I'll leave a comment in PR #4 to make that clear :)

@magajh
Copy link
Contributor

magajh commented Sep 17, 2024

@dcoa left a note in the PR #4 description. What I want is to avoid causing any delays or additional work in this PR beyond what was initially planned because of the refactor that had to be done

Copy link

@bra-i-am bra-i-am left a comment

Choose a reason for hiding this comment

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

@dcoa I think it would be nice to group the logic in steps according to its function (install and configure) instead of its context (nightly or regular version) like:

  - name: Install Tutor
    run: |
      if [ "${{ inputs.tutor_version }}" = "nightly" ]; then
        pip install virtualenv
        virtualenv venv
        source venv/bin/activate
        git clone --branch=nightly --depth=1 https://github.com/overhangio/tutor.git
        pip install -e "./tutor[full]"
      else
        pip install "tutor$INPUT_TUTOR_VERSION"
      fi
    shell: bash

  - name: Configure Tutor
    run: |
      TUTOR_ROOT="$(pwd)"
      $TUTOR_ROOT/tutor --version
      $TUTOR_ROOT/tutor config save
      $TUTOR_ROOT/tutor mounts add lms, cms,lms-worker,cms-worker:$TUTOR_ROOT/${{ inputs.app_name }}:/openedx/${{ inputs.app_name }}
      if [ "${{ inputs.tutor_version }}" = "nightly" ]; then
        $TUTOR_ROOT/tutor images build openedx
      fi
      $TUTOR_ROOT/tutor local launch -I
    shell: bash
    env:
      INPUT_APP: ${{ inputs.app_name }}
      INPUT_TUTOR_VERSION: ${{ inputs.tutor_version }}

I didn't test the code but made it with what I found and thought for illustrative purposes :v... what do you think about it?

@dcoa
Copy link
Contributor Author

dcoa commented Sep 18, 2024

@bra-i-am and @BryanttV I merge both suggestions, could you have a look?

Copy link

@bra-i-am bra-i-am left a comment

Choose a reason for hiding this comment

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

@dcoa, thank you for addressing my comment!

Copy link
Contributor

@BryanttV BryanttV left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@dcoa dcoa merged commit 1fd52b5 into main Sep 18, 2024
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.

4 participants