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

Remove broken "Test unstable" GitHub action #3064

Merged
merged 2 commits into from
Jun 13, 2024

Conversation

ivoanjo
Copy link
Member

@ivoanjo ivoanjo commented Aug 21, 2023

What does this PR do?:

The "Test unstable" GitHub Action runs the dd-trace-rb CI with ruby-head and jruby-head.

Because ruby-head and jruby-head aren't always rock solid, we've marked these tests with "continue-on-error: true" which in practice means you need to actually open them to know if they failed, because they get always marked as succeeded.

Today, while looking into our CI config, I noticed these tests have been broken for what looks like months (due to some refactoring in our gemspec):

[!] There was an error parsing `Gemfile`:
[!] There was an error while loading `ddtrace.gemspec`: syntax error, unexpected ']', expecting `end' or dummy end - ...ERSION::MAXIMUM_RUBY_VERSION}"]
...                              ^
. Bundler cannot continue.

 #  from /home/runner/work/dd-trace-rb/dd-trace-rb/ddtrace.gemspec:10
 #  -------------------------------------------
 #    spec.version               = DDTrace::VERSION::STRING
 >                                  "< #{DDTrace::VERSION::MAXIMUM_RUBY_VERSION}"]
 #    spec.required_rubygems_version = '>= 2.0.0'
 #  -------------------------------------------
. Bundler cannot continue.

 #  from /home/runner/work/dd-trace-rb/dd-trace-rb/Gemfile:3
 #  -------------------------------------------
 #
 >  gemspec
 #
 #  -------------------------------------------
Error: The process '/home/runner/.rubies/ruby-head/bin/bundle' failed with exit code 14

(https://github.com/DataDog/dd-trace-rb/actions/runs/5890935813/job/15977021676)

Nobody seems to notice when they fail, and thus it really looks like we're not getting any value at all.

Thus, I decided to open a PR to remove them for now.

Motivation:

As our CI gets more complex, we get little value from things that need to be checked manually -- nowadays we have hundreds of validations.

Additional Notes:

N/A

How to test the change?:

Validate that CI is still green, and this action no longer runs.

**What does this PR do?**:

The "Test unstable" GitHub Action runs the dd-trace-rb CI with
ruby-head and jruby-head.

Because ruby-head and jruby-head aren't always rock solid, we've marked
these tests with "continue-on-error: true" which in practice means you
need to actually open them to know if they failed, because they get
always marked as succeeded.

Today, while looking into our CI config, I noticed these tests have
been broken for what looks like *months* (due to some refactoring
in our gemspec):

```
[!] There was an error parsing `Gemfile`:
[!] There was an error while loading `ddtrace.gemspec`: syntax error, unexpected ']', expecting `end' or dummy end - ...ERSION::MAXIMUM_RUBY_VERSION}"]
...                              ^
. Bundler cannot continue.

 #  from /home/runner/work/dd-trace-rb/dd-trace-rb/ddtrace.gemspec:10
 #  -------------------------------------------
 #    spec.version               = DDTrace::VERSION::STRING
 >                                  "< #{DDTrace::VERSION::MAXIMUM_RUBY_VERSION}"]
 #    spec.required_rubygems_version = '>= 2.0.0'
 #  -------------------------------------------
. Bundler cannot continue.

 #  from /home/runner/work/dd-trace-rb/dd-trace-rb/Gemfile:3
 #  -------------------------------------------
 #
 >  gemspec
 #
 #  -------------------------------------------
Error: The process '/home/runner/.rubies/ruby-head/bin/bundle' failed with exit code 14
```

(https://github.com/DataDog/dd-trace-rb/actions/runs/5890935813/job/15977021676)

Nobody seems to notice when they fail, and thus it really looks like
we're not getting any value at all.

Thus, I decided to open a PR to remove them for now.

**Motivation**:

As our CI gets more complex, we get little value from things that
need to be checked manually -- nowadays we have hundreds of
validations.

**Additional Notes**:

N/A

**How to test the change?**:

Validate that CI is still green, and this action no longer runs.
@ivoanjo ivoanjo requested a review from a team August 21, 2023 14:32
@github-actions github-actions bot added the dev/github Github repository maintenance and automation label Aug 21, 2023
@codecov-commenter
Copy link

codecov-commenter commented Aug 21, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.10%. Comparing base (66a74aa) to head (b8c91f0).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3064   +/-   ##
=======================================
  Coverage   98.10%   98.10%           
=======================================
  Files        1225     1225           
  Lines       72846    72846           
  Branches     3489     3489           
=======================================
+ Hits        71463    71465    +2     
+ Misses       1383     1381    -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@marcotc marcotc left a comment

Choose a reason for hiding this comment

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

Another victim of the lack of support for https://github.com/orgs/community/discussions/15452

@marcotc
Copy link
Member

marcotc commented Aug 21, 2023

Oh, this looks interesting! https://github.com/mainmatter/continue-on-error-comment

@ivoanjo
Copy link
Member Author

ivoanjo commented Aug 22, 2023

Interesting... Let me see if I can get that action to work.

@ivoanjo
Copy link
Member Author

ivoanjo commented Aug 29, 2023

Update: I think I'm going to take a bit longer to test out the comment thing. On the other hand, having this action failing is harmless. So I guess for now I'll leave this PR open so I don't forget and will come back to it in a couple of weeks :)

@ivoanjo
Copy link
Member Author

ivoanjo commented Oct 27, 2023

Update: This approach seems to work! I've tested it in libdatadog and works nicely: DataDog/libdatadog#280 . I have it on my todo list to come back to this PR and a) fix the broken thingy, and b) use the continue on error comment to remind us when it broke.

Copy link
Contributor

@p-datadog p-datadog left a comment

Choose a reason for hiding this comment

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

+1 for removing failing configurations that are "never" looked at.

With that said, are circle or gitlab better at reporting "allowed failures" than gh?

@ivoanjo
Copy link
Member Author

ivoanjo commented Jun 12, 2024

With that said, are circle or gitlab better at reporting "allowed failures" than gh?

I think most CI providers are a bit sucky at this... (Or at least, I haven't seen a good solution).

@ivoanjo
Copy link
Member Author

ivoanjo commented Jun 12, 2024

TBH I've been sitting on this PR for a long time as I was kinda planning to do the continue on error but haven't done it. Any thoughts on merging as-is vs waiting for that change? cc @p-datadog @marcotc

@p-datadog
Copy link
Contributor

I approve this PR as it is.

@ivoanjo ivoanjo requested a review from a team as a code owner June 13, 2024 07:55
@ivoanjo
Copy link
Member Author

ivoanjo commented Jun 13, 2024

Ok, let's go ahead with this PR. I've refreshed it to remove the conflict, and out of curiosity I checked a recent run of this job and yeap no surprise there's multiple things broken that are getting marked as ✔️ -- https://github.com/DataDog/dd-trace-rb/actions/runs/9485780142/job/26138547233?pr=3711#step:3:1 .

I'll add a separate internal ticket for us to consider reviving this in a better shape, right now we're only really contributing to global warming.

Copy link
Contributor

@TonyCTHsu TonyCTHsu left a comment

Choose a reason for hiding this comment

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

Oh yeah, I won't be looking into this unless it is making noise