Implement GHWorkflow, GHWorkflowRun and associated payloads #1064

gsmet · 2021-03-22T15:12:05Z

This is a draft for now.

I have several questions and there is at least one big issue with the tests.

src/main/java/org/kohsuke/github/GHWorkflowRun.java

src/test/java/org/kohsuke/github/GHWorkflowRunTest.java

gsmet · 2021-03-22T15:17:16Z

@bitwiseman interested with your wisdom on this one. I added some comments for the big remaining issues from my point of view.

pom.xml

src/main/java/org/kohsuke/github/GHRepository.java

gsmet · 2021-03-22T18:25:32Z

@bitwiseman ok, the situation is a bit better: I was able to get the tests to run in both modes.

So that lets us with the 2 design decisions:

enum or not enum for the getStatus/Completion()
isolating the API in workflows()

Let me know your take on these two and I'll adjust.

In the meantime, I will work on the payloads.

bitwiseman · 2021-03-22T19:29:47Z

isolating the API in workflows()

No, it's fine, leave it.

enum or not enum for the getStatus/Completion()

I think you could add a bridge method annotation to bring consistency. And we don't need to create class - we can do the same as EnumUtils.getEnum(Class, String, E). We can't update the version of commons-lang due to downstream limitations, but we can use the same behavior.

I would even be fine with not having a getRaw* method. We simply don't support other values. It would make it very clear that any code depending on this method needs to handle the potential case of unknown values.

Final call is yours. Either is acceptable and can be iterated in a future release.

Most of the actions are implemented but not all. Looks like a good first step.

Provide bridge methods for compatibility.

gsmet · 2021-03-23T14:38:19Z

I think you could add a bridge method annotation to bring consistency. And we don't need to create class - we can do the same as EnumUtils.getEnum(Class, String, E). We can't update the version of commons-lang due to downstream limitations, but we can use the same behavior.

I implemented a method similar to this except it returns null when the value is null.

I used it for GHWorkflowRun and also added a commit to use it in GHCheckRun (with bridge methods). The enums are different as I'm really not sure they are the same in the underlying API. They have the same values for now but who knows.

I just pushed these changes.

I'm going to work on the payloads next, and also take care of the pull requests potentially attached. In passing, I'll fix a bug in GHCheckRun that my colleague @yrodiere discovered recently while working on something unrelated.

Fixes hub4j#1037

gsmet · 2021-03-24T09:41:13Z

@bitwiseman I implemented the payloads in my last commit. From my side, this PR is ready to go. I'll let you review it.

There's still something we need to fix with pull requests for check suites and workflow runs but I'll do it in a separate PR once this is in as it's a preexisting issue and a totally different discussion.

gsmet · 2021-03-24T09:42:49Z

Also, is there a way to test bridge methods?

src/main/java/org/kohsuke/github/GHEventPayload.java

bitwiseman · 2021-03-24T18:07:36Z

Also, is there a way to test bridge methods?

Not really. There is a test to verify they are created, but executing them no. ... I suppose we could add a facility for it using reflection?

bitwiseman · 2021-03-24T18:35:13Z

src/main/java/org/kohsuke/github/internal/EnumUtils.java

+
+public final class EnumUtils {
+
+    public static <E extends Enum<E>> E getEnum(Class<E> enumClass, String value, E defaultEnum) {


JavaDoc needed.

src/main/java/org/kohsuke/github/internal/EnumUtils.java

bitwiseman

Great work on this! The only remaining feedback I have is on tests. Sorry for the multiple rounds, this is the last one and I'll try to do a better job of batching for future PRs.

bitwiseman · 2021-03-24T21:22:54Z

src/test/java/org/kohsuke/github/GHWorkflowTest.java

+        workflow.dispatch("main");
+        workflow.dispatch("main", Collections.singletonMap("parameter", "value"));
+
+        // if we implement the logs API at some point, it might be a good idea to validate all this


With wiremock you can verify that specific requests were made. Would that work for validation?

Do you have an example in the code base?

I haven't used it in this project.
Here's the docs for it: http://wiremock.org/docs/verifying/

Thanks for the pointer. Done.

bitwiseman · 2021-03-25T00:15:31Z

src/test/java/org/kohsuke/github/GHWorkflowRunTest.java

+        Awaitility.await().atLeast(atLeast).pollInterval(pollInterval).atMost(atMost).until(() -> {
+            List<GHWorkflowRun> workflowRuns = getLatestWorkflowRuns(MAIN_BRANCH, Status.IN_PROGRESS);
+            for (GHWorkflowRun workflowRun : workflowRuns) {
+                if (workflowRun.getName().equals(SLOW_WORKFLOW_NAME)
+                        && workflowRun.getId() > latestPreexistingWorkflowRunId) {
+                    assertNotNull(workflowRun.getId());
+
+                    workflowRun.cancel();
+                    cancelledWorkflowRunId = workflowRun.getId();
+                    return true;
+                }
+            }
+            return false;
+        });


A few requests applying to all the await sections:

I'd prefer if the until only polled for the state change and the asserts are called after, outside the until.

Let's use getGitHubBeforeAfter() for polling. That way the tests don't need to record the polling. That way when running CI, the test do effectively no waiting.

With (2) we can remove the special polling interval values in setup(). Maybe use a fibonacci polling interval with milliseconds? Recording will burn a few extra requests but they won't be recorded.

yeah that was my inclination as well first and I thought it would be better to reduce the number of API calls. I changed it. I agree it's more readable this way.

ah, yes, good idea, I was looking for a way to do exactly that

I removed the polling when we don't use a proxy. I tried the fibonacci thingy but it doesn't work very well, don't know why but it was just running forever and I don't really have the cycles to dig more into it. Using a fixed poll interval works so I am sticking with that.

Thanks, looks great. Bummer about the polling interval, but oh well.

We don't record the polling requests and we avoid any polling when not recording.

gsmet · 2021-03-25T10:45:24Z

@bitwiseman I think I addressed all your comments. I would be interested in you releasing a new version once this is in so that I can implement this in my Quarkus GitHub App framework for Jeff!

bitwiseman · 2021-03-25T19:21:14Z

@gsmet Great work! I'll release today.

gsmet · 2021-03-26T08:42:47Z

@bitwiseman thanks for the reviews and release! Glad we could get this one in.

Only execute slow-or-flaky-test if -Dtest= is not defined

fb03e74

gsmet force-pushed the workflow-runs branch from f921b14 to 369b9e3 Compare March 22, 2021 15:13

gsmet commented Mar 22, 2021

View reviewed changes

src/main/java/org/kohsuke/github/GHWorkflowRun.java Outdated Show resolved Hide resolved

gsmet commented Mar 22, 2021

View reviewed changes

src/test/java/org/kohsuke/github/GHWorkflowRunTest.java Outdated Show resolved Hide resolved

bitwiseman reviewed Mar 22, 2021

View reviewed changes

pom.xml Show resolved Hide resolved

bitwiseman reviewed Mar 22, 2021

View reviewed changes

src/main/java/org/kohsuke/github/GHRepository.java Outdated Show resolved Hide resolved

gsmet force-pushed the workflow-runs branch from 369b9e3 to 3be3b62 Compare March 22, 2021 18:18

gsmet added 2 commits March 23, 2021 15:35

Implement GHWorkflow and GHWorkflowRun

042038f

Most of the actions are implemented but not all. Looks like a good first step.

Adjust GHCheckRun so that status and conclusion are returned as enums

b8f00bc

Provide bridge methods for compatibility.

gsmet force-pushed the workflow-runs branch from 3be3b62 to b8f00bc Compare March 23, 2021 14:37

gsmet changed the title ~~Workflow runs~~ Implement GHWorkflow, GHWorkflowRun and associated payloads Mar 23, 2021

Implement GHEventPayload.WorkflowRun and GHEventPayload.WorkflowDispatch

8919c5f

Fixes hub4j#1037

gsmet marked this pull request as ready for review March 24, 2021 09:41

jeffmaury reviewed Mar 24, 2021

View reviewed changes

src/main/java/org/kohsuke/github/GHEventPayload.java Show resolved Hide resolved

bitwiseman reviewed Mar 24, 2021

View reviewed changes

src/main/java/org/kohsuke/github/internal/EnumUtils.java Show resolved Hide resolved

Adjust EnumUtils method name and add javadoc

57cdc30

bitwiseman requested changes Mar 25, 2021

View reviewed changes

gsmet added 2 commits March 25, 2021 11:00

Rearchitecture GHWorkflowRunTest

47e2a5a

We don't record the polling requests and we avoid any polling when not recording.

Add some assertions for GHWorkflow dispatch tests

d734237

bitwiseman approved these changes Mar 25, 2021

View reviewed changes

bitwiseman merged commit 551dbf2 into hub4j:master Mar 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement GHWorkflow, GHWorkflowRun and associated payloads #1064

Implement GHWorkflow, GHWorkflowRun and associated payloads #1064

gsmet commented Mar 22, 2021

gsmet commented Mar 22, 2021

gsmet commented Mar 22, 2021

bitwiseman commented Mar 22, 2021 •

edited

Loading

gsmet commented Mar 23, 2021

gsmet commented Mar 24, 2021

gsmet commented Mar 24, 2021

bitwiseman commented Mar 24, 2021 •

edited

Loading

bitwiseman Mar 24, 2021

gsmet Mar 24, 2021

bitwiseman left a comment

bitwiseman Mar 24, 2021

gsmet Mar 25, 2021

bitwiseman Mar 25, 2021

gsmet Mar 25, 2021

bitwiseman Mar 25, 2021

gsmet Mar 25, 2021

bitwiseman Mar 25, 2021

gsmet commented Mar 25, 2021

bitwiseman commented Mar 25, 2021

gsmet commented Mar 26, 2021


		public final class EnumUtils {

		public static <E extends Enum<E>> E getEnum(Class<E> enumClass, String value, E defaultEnum) {

Implement GHWorkflow, GHWorkflowRun and associated payloads #1064

Implement GHWorkflow, GHWorkflowRun and associated payloads #1064

Conversation

gsmet commented Mar 22, 2021

gsmet commented Mar 22, 2021

gsmet commented Mar 22, 2021

bitwiseman commented Mar 22, 2021 • edited Loading

gsmet commented Mar 23, 2021

gsmet commented Mar 24, 2021

gsmet commented Mar 24, 2021

bitwiseman commented Mar 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitwiseman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsmet commented Mar 25, 2021

bitwiseman commented Mar 25, 2021

gsmet commented Mar 26, 2021

bitwiseman commented Mar 22, 2021 •

edited

Loading

bitwiseman commented Mar 24, 2021 •

edited

Loading