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

Deprecate Go extraction reconciliation feature flag and CODEQL_EXTRACTOR_GO_BUILD_TRACING for custom builds #1322

Merged
merged 36 commits into from
Nov 14, 2022

Conversation

angelapwen
Copy link
Contributor

@angelapwen angelapwen commented Oct 25, 2022

This PR removes the CODEQL_EXTRACTOR_GO_BUILD_TRACING from all tests as well as another unused environment variable, CODEQL_ACTION_RECONCILE_TRACING. Users who previously had CODEQL_EXTRACTOR_GO_BUILD_TRACING set with custom Go builds will be shown a warning suggesting that they remove the environment variable as it no longer has effect.

Additionally, this PR deprecates the use of the Go extraction reconciliation feature flag so that its functionality is always on (ie. Go will always be treated as a traced language). A separate PR removing the flag from dotcom will be merged after this PR.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.
  • Confirm the changelog has been updated if necessary.

@angelapwen angelapwen force-pushed the angelapwen/deprecate-go-env-var branch 2 times, most recently from d65ed09 to 152e246 Compare October 25, 2022 22:04
@angelapwen angelapwen marked this pull request as ready for review October 25, 2022 23:02
@angelapwen angelapwen requested a review from a team as a code owner October 25, 2022 23:03
src/languages.ts Outdated Show resolved Hide resolved
@angelapwen angelapwen changed the title [DRAFT] Deprecate CODEQL_EXTRACTOR_GO_BUILD_TRACING Deprecate CODEQL_EXTRACTOR_GO_BUILD_TRACING Oct 25, 2022
henrymercer
henrymercer previously approved these changes Oct 26, 2022
src/languages.ts Outdated Show resolved Hide resolved
src/languages.ts Outdated Show resolved Hide resolved
@henrymercer
Copy link
Contributor

I'm not sure if the variable deprecation should be part of the changelog here or not 🤔

I think it would make sense to add a changelog note in the PR that removes the feature flag check similar to the one we used before starting rollout, saying that we have now rolled out this change. You could then mention the variable deprecation there too — it won't affect most users but the Actions changelog can be more detailed.

@angelapwen
Copy link
Contributor Author

I'm not sure if the variable deprecation should be part of the changelog here or not 🤔

I think it would make sense to add a changelog note in the PR that removes the feature flag check similar to the one we used before starting rollout, saying that we have now rolled out this change. You could then mention the variable deprecation there too — it won't affect most users but the Actions changelog can be more detailed.

👍 I'll make a note to do so in that PR

@angelapwen angelapwen marked this pull request as draft October 26, 2022 23:16
@angelapwen angelapwen changed the title Deprecate CODEQL_EXTRACTOR_GO_BUILD_TRACING Deprecate Go extraction reconciliation feature flag and CODEQL_EXTRACTOR_GO_BUILD_TRACING Nov 4, 2022
@angelapwen angelapwen force-pushed the angelapwen/deprecate-go-env-var branch 2 times, most recently from 789f7e6 to f69b64d Compare November 4, 2022 23:38
src/languages.ts Outdated Show resolved Hide resolved
@angelapwen angelapwen marked this pull request as ready for review November 5, 2022 00:08
CHANGELOG.md Outdated Show resolved Hide resolved
src/languages.ts Outdated Show resolved Hide resolved
@angelapwen angelapwen force-pushed the angelapwen/deprecate-go-env-var branch 2 times, most recently from 21816d5 to 9011d12 Compare November 7, 2022 20:42
src/init-action.ts Outdated Show resolved Hide resolved
@angelapwen angelapwen mentioned this pull request Nov 7, 2022
3 tasks
@angelapwen angelapwen force-pushed the angelapwen/deprecate-go-env-var branch from 8f1b8c1 to 4d4d91d Compare November 7, 2022 23:19
@angelapwen
Copy link
Contributor Author

I ended up finding an error in the Debug Artifacts Upload PR check because of this change — I hadn't added the actions/setup-go@v3 step in those PR checks so there wasn't any Go source code found for artifact upload. I didn't look into why it was previously passing, but as the PR checks are all passing now I think this is ready for re-review (also pending PM approval of the user-facing warning)

Copy link
Contributor

@henrymercer henrymercer left a comment

Choose a reason for hiding this comment

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

Very nice! Just a couple of comments around the docs.

CHANGELOG.md Outdated Show resolved Hide resolved
src/init-action.ts Outdated Show resolved Hide resolved
@angelapwen angelapwen force-pushed the angelapwen/deprecate-go-env-var branch 2 times, most recently from ad71fd4 to 9f7a92c Compare November 8, 2022 16:39
henrymercer
henrymercer previously approved these changes Nov 8, 2022
Copy link
Contributor

@henrymercer henrymercer left a comment

Choose a reason for hiding this comment

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

LGTM, subject to @AlonaHlobina's approval of the warning message.

@angelapwen angelapwen force-pushed the angelapwen/deprecate-go-env-var branch from 6ad286b to 1fa94b4 Compare November 11, 2022 01:05
@angelapwen
Copy link
Contributor Author

Ok — I resolved some merge conflicts, made some fixes to PR checks that were failing due to the new Go build command being added to build.sh, and re-ran possibly flaky tests. I think this PR is ready for review again — all the "expected" checks are ones that I have removed/replaced as of this change.

henrymercer
henrymercer previously approved these changes Nov 14, 2022
Copy link
Contributor

@henrymercer henrymercer left a comment

Choose a reason for hiding this comment

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

Looks good modulo the CHANGELOG.

CHANGELOG.md Outdated Show resolved Hide resolved
src/analyze-action.ts Show resolved Hide resolved
@angelapwen angelapwen enabled auto-merge (squash) November 14, 2022 20:14
@angelapwen angelapwen merged commit 5883c13 into main Nov 14, 2022
@angelapwen angelapwen deleted the angelapwen/deprecate-go-env-var branch November 14, 2022 21:54
@henrymercer henrymercer mentioned this pull request Nov 15, 2022
3 tasks
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.

3 participants