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

Add Haskell Language Server check to CI #798

Merged
merged 4 commits into from
Jul 9, 2024
Merged

Add Haskell Language Server check to CI #798

merged 4 commits into from
Jul 9, 2024

Conversation

palas
Copy link
Contributor

@palas palas commented Jun 21, 2024

This PR adds a check to the CI that ensures that it works with the developer shell as it is.

Changelog

- description: |
    Add CI check that ensures HLS works with developer shell
  type:
  - test

Context

Recently we had an issue with HLS not working because of a bug. We could have catched that if we had a test in the CI that ensured HLS worked. This PR aims to address this.

How to trust this PR

Check the workflow makes sense. But basically, I would just look at the CI's output.

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated. See Running tests for more details
  • Self-reviewed the diff

@palas palas added enhancement New feature or request test Modifies/extends the test suite labels Jun 21, 2024
@palas palas self-assigned this Jun 21, 2024
@palas palas requested review from a team as code owners June 21, 2024 16:41
@palas palas changed the base branch from fix-hls to main June 21, 2024 16:41
@palas palas force-pushed the add-hls-check-to-ci branch 3 times, most recently from 31d7b9f to a08d7cd Compare June 21, 2024 16:57
@palas palas force-pushed the add-hls-check-to-ci branch 3 times, most recently from 5d262d3 to f850dd7 Compare June 21, 2024 18:48
.github/workflows/hls.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@carbolymer carbolymer left a comment

Choose a reason for hiding this comment

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

I'm fine with the version bump. This GHA however takes 20 minutes, which is too long of a wait for each PR to get merged.

I don't remember, can you confirm that each PR will wait for this new check to complete before merging?

@smelc
Copy link
Contributor

smelc commented Jun 25, 2024

@carbolymer> right now the new check is not required so we can merge manually before this new check is finished.

Regarding the merge queue's behavior, the documentation says it waits for required checks to be satisfied. So not sure if it merges as soon as required checks are green, ignoring the others. I'd be surprised if it was the case.

@palas palas force-pushed the add-hls-check-to-ci branch 5 times, most recently from 119da45 to 20f12c2 Compare June 25, 2024 15:47
@palas
Copy link
Contributor Author

palas commented Jun 25, 2024

I'm fine with the version bump. This GHA however takes 20 minutes, which is too long of a wait for each PR to get merged.

I don't remember, can you confirm that each PR will wait for this new check to complete before merging?

It takes 4 min now on cache hit. And, it is my understanding, that it should hit cache as long as dependencies for the project don't change

hls-cached

@palas palas requested a review from carbolymer June 25, 2024 21:01
Copy link
Contributor

@carbolymer carbolymer left a comment

Choose a reason for hiding this comment

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

If it takes 4 mins, I'm fine with it! 🚀

@palas palas force-pushed the add-hls-check-to-ci branch 2 times, most recently from f0cf09a to c530d9a Compare July 8, 2024 17:24
@palas palas force-pushed the add-hls-check-to-ci branch 2 times, most recently from 62903e1 to d24b4f9 Compare July 8, 2024 17:59
@palas palas changed the base branch from main to fix-hls July 8, 2024 18:49
@palas palas force-pushed the add-hls-check-to-ci branch 2 times, most recently from 23300c7 to a5c44a6 Compare July 9, 2024 16:43
@palas palas removed the DO NOT MERGE label Jul 9, 2024
Base automatically changed from fix-hls to main July 9, 2024 19:08
@palas palas added this pull request to the merge queue Jul 9, 2024
@palas palas removed this pull request from the merge queue due to a manual request Jul 9, 2024
@palas palas added this pull request to the merge queue Jul 9, 2024
Merged via the queue into main with commit 67a3dfe Jul 9, 2024
24 checks passed
@palas palas deleted the add-hls-check-to-ci branch July 9, 2024 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request test Modifies/extends the test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants