-
-
Notifications
You must be signed in to change notification settings - Fork 15
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
Run e2etests on release #83
Conversation
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.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Enkidu93 and @johnml1135)
.github/workflows/ci-e2e.yml
line 7 at r1 (raw file):
branches: ["run_e2etests_on_release_#100"] release: types: [published]
I don't think we are currently publishing releases. Right now, we mark releases with tags.
src/SIL.Machine.AspNetCore/Services/SmtTransferEngineBuildJob.cs
line 100 at r1 (raw file):
{ cancellationToken.ThrowIfCancellationRequested(); await smtModelTrainer.SaveAsync(CancellationToken.None);
I'm confused. Wasn't this already fixed in another PR?
Previously, ddaspit (Damien Daspit) wrote…
@Enkidu93, can you rebase this off of master? That should clear up these artifacts. |
|
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.
Reviewed all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Enkidu93)
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.
Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @ddaspit and @johnml1135)
Previously, johnml1135 (John Lambert) wrote…
Can you rebase and clean up the history to be 1 or 2 commits?
OK! I can just squash and merge when the PR is approved, right?
.github/workflows/ci-e2e.yml
line 7 at r1 (raw file):
Previously, ddaspit (Damien Daspit) wrote…
I don't think we are currently publishing releases. Right now, we mark releases with tags.
Done.
src/SIL.Machine.AspNetCore/Services/SmtTransferEngineBuildJob.cs
line 100 at r1 (raw file):
Previously, johnml1135 (John Lambert) wrote…
@Enkidu93, can you rebase this off of master? That should clear up these artifacts.
I think I've fixed this.
Previously, Enkidu93 (Eli C. Lowry) wrote…
The main release tag that would care about E2E tests is "docker_*". |
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.
Reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @ddaspit)
Previously, Enkidu93 (Eli C. Lowry) wrote…
I see that you rebased - but also merged the changes in over time. I believe that @ddaspit prefers if possible for each branch to branch off of master, and be "recreated" as a commit or 2 rebased onto master. |
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.
Reviewed 2 of 2 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ddaspit)
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.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ddaspit and @Enkidu93)
.github/workflows/ci-e2e.yml
line 8 at r2 (raw file):
- 'v*.*.*' workflow_dispatch:
should be changed to docker_*
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.
Reviewable status: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @ddaspit and @johnml1135)
Previously, johnml1135 (John Lambert) wrote…
I see that you rebased - but also merged the changes in over time. I believe that @ddaspit prefers if possible for each branch to branch off of master, and be "recreated" as a commit or 2 rebased onto master.
Yeah, I apologize. I did not do what I intended, but then I realized that I believe I can do what you're asking in the PR on GitHub when I merge (both the proper rebase and the squash). I'm sorry.
.github/workflows/ci-e2e.yml
line 7 at r1 (raw file):
Previously, johnml1135 (John Lambert) wrote…
The main release tag that would care about E2E tests is "docker_*".
Done.
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.
Reviewed 1 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Enkidu93)
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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @johnml1135)
.github/workflows/ci-e2e.yml
line 8 at r2 (raw file):
Previously, johnml1135 (John Lambert) wrote…
should be changed to docker_*
Done.
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.
Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Enkidu93)
Fixes sillsdev/serval#100. (Currently failing E2E tests are resolved elsewhere).
This change is