-
Notifications
You must be signed in to change notification settings - Fork 337
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
Gh actions - first pass #750
Conversation
-removed travis
-logging handlers don't actually raies exceptions, see https://docs.python.org/2/howto/logging.html#exceptions-raised-during-logging -...we could get them to, but at this point I think it's too much complexity for a trivial case
-this causes return values to be strs rather than byte streams -this was causing the decode('utf-8') error, not py3.6/7 as previously thought
They will once you merge the changes into |
Btw are you planning to squash the history? |
Bam! Of course I need a merge to master first. I'm not sure if there's even
an option for branch-specific badges yet either, there's no mention in the
docs.
History - you mean the run history..? Not even sure if that's possible yet.
…On Fri, Sep 27, 2019 at 3:29 PM Thomas Mansencal ***@***.***> wrote:
Btw are you planning to squash the history?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#750>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAMOUSVTSFIPE2JZF6A6DJ3QLWK3LANCNFSM4I3BMN2A>
.
|
Ah right gotcha. Yeah I'll make sure I do that before merge. Had to keep
pushing in order to test.
A
…On Fri, Sep 27, 2019 at 3:39 PM Thomas Mansencal ***@***.***> wrote:
History - you mean the run history..? Not even sure if that's possible yet.
Nah, the commit one, it scorched my retina! :)
[image: image]
<https://user-images.githubusercontent.com/99779/65744808-aa747100-e14d-11e9-8720-87e6d894a734.png>
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#750>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAMOUSRUHK4N2YE7BKA6PI3QLWL73ANCNFSM4I3BMN2A>
.
|
Sure absolutely make sense, something I noticed before but they fixed during the last few days is that file parsing errors were creating junk on the left actions sidebar. It is gone now, for the better :) |
Yeah I've noticed other stuff too. In the Actions tab, the available runs
on the left-hand side appear and disappear at odd times, and sometimes
you're not in the run you'd expect. Definitely still beta. But I bet it's
gonna get so much better. I'm a bit surprised the workflow language is so
limited though, I've already hit a few cases where something like Jinja2
would be really useful.
…On Fri, Sep 27, 2019 at 3:47 PM Thomas Mansencal ***@***.***> wrote:
Sure absolutely make sense, something I noticed before but they fixed
during the last few days is that file parsing errors were creating junk on
the left actions sidebar. It is gone now, for the better :)
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#750>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAMOUSS2QXHLV7Y3KR4LVHDQLWNAVANCNFSM4I3BMN2A>
.
|
It is almost exactly the Azure Pipelines stuff, and yeah there are some very frustrating bits! It will get better though, can see things changing on a per-day basis almost! |
I'm wondering, should we test on CentOS instead of Ubuntu as CentOS is probably way more used than Ubuntu in VFX? |
Well screw my last comment, they don't provide anything other than Ubuntu :( . |
PowerShell (5.x) and cmd should be build in Windows. We just need pwsh (PowerShellCore 6.x): EDIT: Better docs https://docs.microsoft.com/en-us/powershell/scripting/install/installing-powershell-core-on-windows?view=powershell-6 |
Worth checking if pwsh is not installed by default on all platforms: If not something like might do the trick (just tested locally):
|
Notes:
A couple of test-related things were changed:
per_available_shell
.**Popen_args
instead anyway.One thing we should definitely do is port to pytest and do away with the per_available_shell silliness. I'd be interested in adding a
--test
option toinstall.py
, which would then install the rez-selftest tool, and pytest (as a native requirement in the installation venv, not a vendored lib). I think it'd be better to not install rez-selftest if--test
wasn't specified, since it would require pytest in order to work.Another thing we should do is update the rez-selftest
-s
arg to take multiple shells, and for each of those, to assert that the shell is available, rather than skip over it if not. Right now it isn't that clear whether a given shell is actually getting tested in the CI; for eg, rez could think the shell is unavailable (which in itself may be a bug). So the way I'd see it working is, we would do the apt-get install etc of the shells we want to test, then pass those shells to rez-selftest also, which would fail if any of those shells are not available / rez doesn't know about them.