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 best practices for tooling #551

Merged
merged 8 commits into from
Aug 7, 2024
Merged

Conversation

ErikSchierboom
Copy link
Member

No description provided.

@ErikSchierboom ErikSchierboom marked this pull request as draft August 2, 2024 14:02
@ErikSchierboom ErikSchierboom force-pushed the tooling-best-practices branch from ca607d7 to 71055a0 Compare August 2, 2024 14:02
@ErikSchierboom ErikSchierboom force-pushed the tooling-best-practices branch from 71055a0 to 936b93c Compare August 2, 2024 14:03
@SleeplessByte
Copy link
Member

What's the difference between ## Timeouts (10s cut-off with 408 Request timeout) and ## Configuration (20s cut-off with timeout)? Is there a way to actually get results.out? Is there a way to inspect the ops_error of a run? In which cases are there ops_errors? (I assume these are different from the 408 and 413 etc)

@kmarker1101
Copy link

Along with pinning the base image version as suggested, I wonder if we should standardize on a specific versions for alpine, ubuntu etc. My thought was that when we upgrade, we could then use a bot to check and create issues across all repo's to perform upgrade.

building/tooling/best-practices.md Outdated Show resolved Hide resolved
building/tooling/best-practices.md Outdated Show resolved Hide resolved
building/tooling/best-practices.md Outdated Show resolved Hide resolved
building/tooling/best-practices.md Outdated Show resolved Hide resolved
building/tooling/best-practices.md Outdated Show resolved Hide resolved
building/tooling/best-practices.md Outdated Show resolved Hide resolved
building/tooling/best-practices.md Outdated Show resolved Hide resolved
building/tooling/best-practices.md Outdated Show resolved Hide resolved
building/tooling/best-practices.md Outdated Show resolved Hide resolved
Co-authored-by: András B Nagy <20251272+BNAndras@users.noreply.github.com>
@ErikSchierboom
Copy link
Member Author

Along with pinning the base image version as suggested, I wonder if we should standardize on a specific versions for alpine, ubuntu etc. My thought was that when we upgrade, we could then use a bot to check and create issues across all repo's to perform upgrade.

I don't think we will get much from that to be honest. We already use dependabot to create PRs to update dependencies to the latest version, but tracks should still feel free to pin to a specific version (e.g. because a later version breaks things).

@SleeplessByte
Copy link
Member

You may want to add that the tooling should work with both .meta (when testing against the GitHub repo) and .exercism (when testing against actual solutions).

@ErikSchierboom
Copy link
Member Author

You may want to add that the tooling should work with both .meta (when testing against the GitHub repo) and .exercism (when testing against actual solutions).

That's not entirely true though. .exercism only exists locally, when someone uses the CLI to download an exercise. Tooling won't ever see that directory though.

@ErikSchierboom
Copy link
Member Author

What's the difference between ## Timeouts (10s cut-off with 408 Request timeout) and ## Configuration (20s cut-off with timeout)?

Fixed

Is there a way to actually get results.out?
Nope

Is there a way to inspect the ops_error of a run?
Nope

In which cases are there ops_errors? (I assume these are different from the 408 and 413 etc)
We have the following statuses:

TIMEOUT_STATUS = 408
DID_NOT_EXECUTE_STATUS = 410
EXCESSIVE_STDOUT_STATUS = 413
EXCESSIVE_OUTPUT_STATUS = 460
FAILED_TO_PREPARE_INPUT = 512
UNKNOWN_ERROR_STATUS = 513

The DID_NOT_EXECUTE_STATUS status happens when somehow we can't start the process.
The FAILED_TO_PREPARE_INPUT status happens when we can't setup the files/directories for the solution to be processed

@ErikSchierboom ErikSchierboom marked this pull request as ready for review August 7, 2024 09:18
@ErikSchierboom
Copy link
Member Author

I'm gonna merge this. We can tweak later!

@ErikSchierboom ErikSchierboom merged commit 6ca912f into main Aug 7, 2024
1 check passed
@ErikSchierboom ErikSchierboom deleted the tooling-best-practices branch August 7, 2024 09:18
@SleeplessByte
Copy link
Member

You may want to add that the tooling should work with both .meta (when testing against the GitHub repo) and .exercism (when testing against actual solutions).

That's not entirely true though. .exercism only exists locally, when someone uses the CLI to download an exercise. Tooling won't ever see that directory though.

Sure, but it is something I have ran into. Making docker work, and then the same internal commands failing on a downloaded solution. I think it's worth a mention in all the related places because it's unepected.

@SleeplessByte
Copy link
Member

The DID_NOT_EXECUTE_STATUS status happens when somehow we can't start the process. The FAILED_TO_PREPARE_INPUT status happens when we can't setup the files/directories for the solution to be processed

Nice, that helps.

Is there a way to receive the 500 as seen in https://forum.exercism.org/t/typescript-test-runner-ops-error/12353? Because according to these docs that should never happen.

@ErikSchierboom
Copy link
Member Author

Is there a way to receive the 500

What do you mean with "receive"?

@SleeplessByte
Copy link
Member

SleeplessByte commented Aug 7, 2024

See, read, download, explore, "get" the actual error (perhaps with stacktrace, if any).

"status": "ops_error",
"message": "An unknown error occurred",
"message_html": "An unknown error occurred",

This is not debuggable for us.

@ErikSchierboom
Copy link
Member Author

Not right now, no

@SleeplessByte
Copy link
Member

SleeplessByte commented Aug 8, 2024

Not right now, no

imo this (that it is not possible) is valuable to know! 🙂

@ErikSchierboom
Copy link
Member Author

I'll add it to the docs.

@ErikSchierboom
Copy link
Member Author

#557

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.

4 participants