-
Notifications
You must be signed in to change notification settings - Fork 165
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
Use pkl to generate gha workflows #3603
Conversation
Pull Request Test Coverage Report for Build 9199382207Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
* main: RNET-1134: Add obfuscation attribute on generated RealmSchema (#3594) Revert "Hide collections in mixed from public API" (#3606) Update to Core 14.7.0 (#3605) RNET-1145, RNET-1143: Fix reading schema from native (#3601) # Conflicts: # .github/templates/test-weaver.yml # .github/workflows/test-weaver.yml
@@ -1125,7 +1125,7 @@ private async Task<ContainerInfo> WaitForContainer(string containerId, int maxRe | |||
await Task.Delay(2000); | |||
} | |||
|
|||
throw new Exception($"Container with id={containerId} was not found or ready after {maxRetries} retrues"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why aren't we keeping maxRetries
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we're actually decrementing maxRetries
, so if we get here, maxRetries
will always be 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, as we said, maybe it makes sense to save the original maxRetries value ;)
|
||
## Building the workflows | ||
|
||
```bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably put this in a script
+ uwpArchs.map((arch) -> "windows-uwp-\(arch)") | ||
+ applePlatformTargets((platform, target) -> "\(platform)-\(target)") | ||
|
||
const defaultEnv: Mapping<String, String | Boolean> = new { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is having String | Boolean
here only for future-proofing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's matching the type of env
.
if-no-files-found: error | ||
- if: matrix.os == 'ubuntu' | ||
run: git clean -fdx | ||
strategy: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way of putting this before all the other steps as it was before? I thought this was missing at a first glance
/// Runs your workflow when someone creates or updates a Wiki page, which triggers the gollum event. | ||
/// | ||
/// https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#gollum | ||
class Gollum extends Trigger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have given a look around and it mostly made sense, even though it's a dense PR.
I think I'm just a little bit torn on the fact that we know have fewer but huge workflows instead of having them split in smaller ones. Hopefully with pkl it should be easier to manage those though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have given a look around and it mostly made sense, even though it's a dense PR.
I think I'm just a little bit torn on the fact that we know have fewer but huge workflows instead of having them split in smaller ones. Hopefully with pkl it should be easier to manage those though
No description provided.