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

feat(cli): Support GitHub Workflow Commands #681

Merged
merged 11 commits into from
Dec 4, 2023
Merged

feat(cli): Support GitHub Workflow Commands #681

merged 11 commits into from
Dec 4, 2023

Conversation

nikeee
Copy link
Contributor

@nikeee nikeee commented Nov 5, 2023

Summary

See discussion in #441

WIP

Open questions / TODO:

  • Where to put the env var check? Maybe add a field to TraversalMode::CI that states if the cli is running in some CI (determined by the env vars).
  • File name escapes

@github-actions github-actions bot added A-CLI Area: CLI A-Diagnostic Area: diagnostocis labels Nov 5, 2023
@ematipico
Copy link
Member

ematipico commented Nov 6, 2023

Where to put the env var check? Maybe add a field to TraversalMode::CI that states if the cli is running in some CI (determined by the env vars).

I think so. We could add something like

    CI {
        /// Whether the CI is running in a specific environment, e.g. GitHub, GitLab, etc.
        environment_name: Option<String>
    },

We could create a new function to read the environment variable:

    pub(crate) fn new_ci() -> Self {
        let environment_name = std::option_env!("GITHUB_ACTIONS").and_then(|value| {
            Some(String::from(value))
        });
        Self {
            report_mode: ReportMode::default(),
            traversal_mode: TraversalMode::CI {
                environment_name
            },
            max_diagnostics: MAXIMUM_DISPLAYABLE_DIAGNOSTICS,
        }
    }

This will be called in the ci.rs file

  • File name escapes

I'm not sure what you mean; can you explain?

@nikeee
Copy link
Contributor Author

nikeee commented Nov 6, 2023

Thanks for the feedback. I've updated the PR.

However, I created an enum for the known environments (since we have to implement them, we can also name them):

pub(crate) enum ExecutionEnvironment {
    GitHub,
}

I also used std::env::var instead of std::option_env! because the latter is a compile-time check and we seem to be in the need of a runtime check here.

I'm not sure what you mean; can you explain?

As the workflow commands are just "CSV" 1, we need to escape , in file names, so the fields/files are not mixed up. At least that's what I thought.
I've looked in the documentation as well as the core library source and the actions runner source + the parsing tests and did not find any indication of escaping or quoting. Maybe we can just omit this and assume that the file name does not contain a ,.
Edit: Oh it does. It needs some URL encoding.
Edit 2: Implemented in f50cced

I still haven't managed to get the diagnostic description due to the printer function using a biome_console Formatter and the diagnostic.description(fmt) expecting an std formatter.

We also need to set the env var GITHUB_ACTIONS in CI, so this feature doesn't break the current tests (since it emits more than before ;)). Setting it to false should work. Or we just update the snapshots.

Footnotes

  1. "::{} file={},line={},endLine={},col={},endColumn={}::{}"

@nikeee
Copy link
Contributor Author

nikeee commented Nov 23, 2023

@ematipico the tests are failing due to the workflow commands being emitted because the whole test suite is executed in GH Actions.
Should we accept these as baseline or set GITHUB_ACTIONS to false in CI tests?
Accepting them as a baseline will likely make it inconvenient to run the tests locally.

@ematipico
Copy link
Member

@ematipico the tests are failing due to the workflow commands being emitted because the whole test suite is executed in GH Actions. Should we accept these as baseline or set GITHUB_ACTIONS to false in CI tests? Accepting them as a baseline will likely make it inconvenient to run the tests locally.

We should disable this functionality in the snapshot tests; as you said, it would create a lot of friction. Although it would be great if we could have some assertions before printing the snapshot, to make sure that the output is correct.

We can use the redact_snapshot function inside snap_test.rs to remove that message from the final snapshot.

@nikeee
Copy link
Contributor Author

nikeee commented Nov 25, 2023

@ematipico I've added it to the redaction in a9d5572. There are cases where a workflow command is emitted without a "normal" diagnostic message, which results in the output being empty. To match against current snapshots, I've refactored the method to return an Option<T> and skipped the message as if it wasn't there to begin with.

@nikeee nikeee marked this pull request as ready for review November 25, 2023 18:45
@nikeee nikeee requested a review from ematipico December 4, 2023 10:29
Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

I am delighted to have this feature and merge it! I am sure that we will need more changes, like setting up a github token/permission, because the github action will attempt to write something in CI.

I plan to release a nightly tomorrow and start testing it on other repositories

@ematipico ematipico merged commit 5505a1a into biomejs:main Dec 4, 2023
12 checks passed
@nikeee nikeee deleted the github-warnings branch December 4, 2023 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CLI Area: CLI A-Diagnostic Area: diagnostocis
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants