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

Actions #128

Merged
merged 7 commits into from
May 10, 2022
Merged

Actions #128

merged 7 commits into from
May 10, 2022

Conversation

jburel
Copy link
Member

@jburel jburel commented May 4, 2022

This PR partially re-activates the check via GHA
More clean up to be done
See https://github.com/jburel/omego/actions/runs/2269147023

@jburel jburel closed this May 4, 2022
@jburel jburel reopened this May 4, 2022
Copy link
Member

@joshmoore joshmoore left a comment

Choose a reason for hiding this comment

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

Few questions, but the changes generally look fine. It looks, however, as if the build has hung.

fail-fast: false
matrix:
python-version:
- '3.6'
Copy link
Member

Choose a reason for hiding this comment

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

A side note: python 3.6 is still listed in the docs, but this is something we may want to re-evaluate. See ome/omero-demo-cleanup#6 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see it got bumped later. 👍

uses: actions/setup-java@v1
with:
java-version: 11
- name: Install slice2java
Copy link
Member

Choose a reason for hiding this comment

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

I again note that some of this would be better refactored either into a re-usable workflow or into a script somewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes.If we are okay with the name of the repo I have put in place of re-usable workflows (https://github.com/jburel/action-workflows) I could start refactoring

Copy link
Member

Choose a reason for hiding this comment

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

ome/action-workflows seems fine. Worst case we have to re-adjust in the future.

Copy link
Member Author

@jburel jburel May 5, 2022

Choose a reason for hiding this comment

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

Actually I think that in that case we will need to use composite action e.g. https://github.com/jburel/action-workflows/blob/main/action.yml
So the repository mentioned is not going to work since we will need one repo per composite action i.e. something similar to ansible roles.

@@ -45,19 +45,22 @@ def setup_class(self):
self.branch = 'OMERO-DEV-latest'
self.ice = '3.6'

@pytest.mark.skipif(True, reason='URL to be updated')
Copy link
Member

Choose a reason for hiding this comment

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

What does "URL to be updated" mean?

Copy link
Member Author

Choose a reason for hiding this comment

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

I meant: Change URL since it uses the old ci

@@ -59,8 +59,7 @@ def testUpgrade(self):
self.upgrade(*args)

@pytest.mark.slowtest
@pytest.mark.skipif(True,
reason='OMERO not supported on Python 3.6')
@pytest.mark.skipif(True, reason='OMERO not supported on Python 3.6')
Copy link
Member

Choose a reason for hiding this comment

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

Does this still hold if you've bumped to Py38?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have not re-activated the tests in this PR. I have actually skipped a good number due to URL.
This will be a second body of work

Copy link
Member

Choose a reason for hiding this comment

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

I have not re-activated the tests in this PR

Fair enough. The comment might just be slightly confusing.

@jburel
Copy link
Member Author

jburel commented May 5, 2022

The build hangs in travis not GHA related.

@joshmoore
Copy link
Member

The build hangs in travis not GHA related.

Ok, probably needs to be de-activated, but why isn't your workflow showing up?

@jburel
Copy link
Member Author

jburel commented May 5, 2022

This is a limitation of GHA, it will show up when it is merged.

@jburel
Copy link
Member Author

jburel commented May 5, 2022

@jburel
Copy link
Member Author

jburel commented May 6, 2022

@@ -39,7 +39,7 @@ jobs:
steps:
- uses: actions/checkout@v2
- name: Install Ice Java and Python binding
uses: jburel/action-ice@v1
uses: ome/action-ice@v1
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member Author

Choose a reason for hiding this comment

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

@joshmoore
Copy link
Member

Anyone still have memories of how to disconnect travis?!

@jburel jburel closed this May 6, 2022
@jburel jburel reopened this May 6, 2022
@sbesson
Copy link
Member

sbesson commented May 6, 2022

Anyone still have memories of how to disconnect travis?!

Done. For the record, https://github.com/ome/omego/settings/branches, then Edit the branch and review the Require status checks to pass before merging section, removing stale status checks as needed

@jburel jburel closed this May 6, 2022
@jburel jburel reopened this May 6, 2022
@joshmoore
Copy link
Member

Thanks, guys. Still seems that the workflow isn't running.

@jburel
Copy link
Member Author

jburel commented May 7, 2022

It will only be running when it is merged in master. Unfortunately it has to be validated with my account cf. links of actions run

@sbesson
Copy link
Member

sbesson commented May 10, 2022

It will only be running when it is merged in master. Unfortunately it has to be validated with my account cf. links of actions run

Not necessarily for this PR but I imagine pushing a stub no-op workflow to master and opening a PR to modify the workflow logic could workaround this security limitation.

@jburel
Copy link
Member Author

jburel commented May 10, 2022

It will only be running when it is merged in master. Unfortunately it has to be validated with my account cf. links of actions run

Not necessarily for this PR but I imagine pushing a stub no-op workflow to master and opening a PR to modify the workflow logic could workaround this security limitation.

That's true

@jburel
Copy link
Member Author

jburel commented May 10, 2022

@joshmoore @sbesson i applied the suggestion from @sbesson i.e. pushed an empty workflow to master and modified in this PR.
Action has been run and is green

@joshmoore
Copy link
Member

Thanks for slogging through this, @jburel! Very happy to start rolling the actions to other repos.

@joshmoore joshmoore merged commit 1d509ca into ome:master May 10, 2022
@joshmoore
Copy link
Member

Leaving you to capture the test re-activation(s) how you see best.

@jburel jburel deleted the actions branch May 11, 2022 08:40
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