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

[#768] improvement(CI): Avoiding related issues by changing CI pipeline dependencies #1500

Closed
wants to merge 3 commits into from

Conversation

qqqttt123
Copy link
Contributor

@qqqttt123 qqqttt123 commented Jan 15, 2024

What changes were proposed in this pull request?

Setting the job dependencies. There are some issues if we use two workflows. I change them to one workflow.

Why are the changes needed?

Fix: #768

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Tested in my repo.
qqqttt123#4

@qqqttt123 qqqttt123 changed the title [#768] improvement(CI): Setting the workflow dependencies [#768] improvement(CI): Setting the job dependencies Jan 15, 2024

- name: Package Gravitino
run: |
./gradlew build -x test -PjdkVersion=${{ matrix.java-version }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this step still required? As we've already build the project above.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the job name is 'Package Gravitino' and it is indeed a packaging task, the build task can be skipped and use compileDistribution directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

Copy link
Contributor Author

@qqqttt123 qqqttt123 Jan 16, 2024

Choose a reason for hiding this comment

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

I tried. But It will fail. I recover the command.

Copy link
Contributor

@yuqi1129 yuqi1129 Jan 16, 2024

Choose a reason for hiding this comment

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

I execute ./gradlew compileDistribution -x test locally without executing ./gradlew build, and it seems no error exists

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I wander why ./gradlew compileDistribution -x test will trigger integration test...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wander why ./gradlew compileDistribution -x test will trigger integration test...

We have next steps. The step which you mentioned doesn't trigger.

Copy link
Contributor

Choose a reason for hiding this comment

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

We have already built the Gravitino in line 85, I was thinking why can't we reuse the built package?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They're different jobs. Different jobs may be scheduled to different machines.

@yuqi1129
Copy link
Contributor

I recommend changing the title to "Avoiding Related Issues by Changing CI Pipeline Dependencies" or something similar.

@qqqttt123 qqqttt123 changed the title [#768] improvement(CI): Setting the job dependencies [#768] improvement(CI): Avoiding related issues by Changing CI pipeline dependencies Jan 16, 2024
@qqqttt123 qqqttt123 changed the title [#768] improvement(CI): Avoiding related issues by Changing CI pipeline dependencies [#768] improvement(CI): Avoiding related issues by changing CI pipeline dependencies Jan 16, 2024
@qqqttt123
Copy link
Contributor Author

I recommend changing the title to "Avoiding Related Issues by Changing CI Pipeline Dependencies" or something similar.

done.

@jerryshao
Copy link
Contributor

One thing I was thinking is what is the benefit of this change? I can see the total CI time will be increased because we make it in sequence, with the cost of it what was the actual benefit?

@qqqttt123
Copy link
Contributor Author

One thing I was thinking is what is the benefit of this change? I can see the total CI time will be increased because we make it in sequence, with the cost of it what was the actual benefit?

We can save the resource of Github action. That's a good point. But for CI time, this is not a good choice.

@qqqttt123 qqqttt123 closed this Jan 16, 2024
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.

[Improvement] Setting the workflow depends
3 participants