Skip to content

ref(core): Replace release detection with Sentry CLI release detection #87

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Nov 7, 2022
Merged

ref(core): Replace release detection with Sentry CLI release detection #87

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b4bfb58
ref(core): Replace release detection with Sentry CLI release detection
Lms24 Nov 4, 2022
048a5ef
fix test
Lms24 Nov 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 0 additions & 42 deletions packages/bundler-plugin-core/src/detect-release.ts
View file
Open in desktop

This file was deleted.

15 changes: 11 additions & 4 deletions packages/bundler-plugin-core/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import { addSpanToTransaction, captureMinimalError, makeSentryClient } from "./s
import { Span, Transaction } from "@sentry/types";
import { createLogger } from "./sentry/logger";
import { normalizeUserOptions } from "./options-mapping";
import { getSentryCli } from "./sentry/cli";

// We prefix the polyfill id with \0 to tell other plugins not to try to load or transform it.
// This hack is taken straight from https://rollupjs.org/guide/en/#resolveid.
Expand Down Expand Up @@ -87,6 +88,8 @@ const unplugin = createUnplugin<Options>((options, unpluginMetaContext) => {
silent: internalOptions.silent,
});

const cli = getSentryCli(internalOptions, logger);

if (internalOptions.telemetry) {
logger.info("Sending error and performance telemetry data to Sentry.");
logger.info("To disable telemetry, set `options.telemetry` to `false`.");
Expand Down Expand Up @@ -116,13 +119,17 @@ const unplugin = createUnplugin<Options>((options, unpluginMetaContext) => {
/**
* Responsible for starting the plugin execution transaction and the release injection span
*/
buildStart() {
async buildStart() {
if (!internalOptions.release) {
internalOptions.release = await cli.releases.proposeVersion();
}
Comment on lines +123 to +125
Copy link
Member Author

@Lms24 Lms24 Nov 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

In case we don't have a release (i.e. an empty string), we use the CLI to detect it automatically


transaction = sentryHub.startTransaction({
op: "function.plugin",
name: "Sentry Bundler Plugin execution",
});
releaseInjectionSpan = addSpanToTransaction(
{ hub: sentryHub, parentSpan: transaction, logger },
{ hub: sentryHub, parentSpan: transaction, logger, cli },
"function.plugin.inject_release",
"Release injection"
);
Expand Down Expand Up @@ -259,7 +266,7 @@ const unplugin = createUnplugin<Options>((options, unpluginMetaContext) => {
const releasePipelineSpan =
transaction &&
addSpanToTransaction(
{ hub: sentryHub, parentSpan: transaction, logger },
{ hub: sentryHub, parentSpan: transaction, logger, cli },
"function.plugin.release",
"Release pipeline"
);
Expand All @@ -275,7 +282,7 @@ const unplugin = createUnplugin<Options>((options, unpluginMetaContext) => {
// That's good for them but a hassle for us. Let's try to normalize this into one data type
// (I vote IncludeEntry[]) and continue with that down the line

const ctx: BuildContext = { hub: sentryHub, parentSpan: releasePipelineSpan, logger };
const ctx: BuildContext = { hub: sentryHub, parentSpan: releasePipelineSpan, logger, cli };

createNewRelease(internalOptions, ctx)
.then(() => cleanArtifacts(internalOptions, ctx))
Expand Down
3 changes: 1 addition & 2 deletions packages/bundler-plugin-core/src/options-mapping.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { detectRelease } from "./detect-release";
import { IncludeEntry as UserIncludeEntry, Options as UserOptions } from "./types";

type RequiredInternalOptions = Required<
Expand Down Expand Up @@ -81,7 +80,7 @@ export function normalizeUserOptions(userOptions: UserOptions): InternalOptions
project: userOptions.project,
authToken: userOptions.authToken,
url: userOptions.url ?? "https://sentry.io/",
release: userOptions.release ?? detectRelease(),
release: userOptions.release ?? "",
Copy link
Member Author

@Lms24 Lms24 Nov 4, 2022

Choose a reason for hiding this comment

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

This seems odd but I want to keep release a required property, hence I'm assigning an empty string.

Copy link

@lforst lforst Nov 4, 2022

Choose a reason for hiding this comment

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

Would it hurt us to make it an optional property? If it is actually optional now, we should define it as such. (Same probably down the line for project, authToken etc. because they could be defined via config/env-vars now right?)

Copy link
Member Author

@Lms24 Lms24 Nov 4, 2022

Choose a reason for hiding this comment

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

Well, we'd have to check for its definedness everywhere while right now we could be sure that it's defined. For env vars (whenever we bring them back) we can just assign them in our normalization step so we could still leave token, org, project, etc required.

Generally, I'm thinking of moving our validation logic (which def needs to be refactored) into or before the final internal option conversion step so we can error before the plugin does anything. WDYT, does this sound reasonable?

Copy link
Member Author

@Lms24 Lms24 Nov 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

release is a special case here because we can't easily await the cli.releases.proposeVersion promise in the top-level function we pass to createUnplugin. Hence I moved it to the buildStart hook

Copy link
Member Author

@Lms24 Lms24 Nov 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

because they could be defined via config/env-vars now right?

@lforst especially the config file is a valid point. Haven't thought about this when making the change here. So this means that, unless we want to read and parse the config file during normalization (which I don't want to do unless necessary), we have to refactor our internal options conversion to actually make these parameters (releaes, authtoken, etc) optional again...

I'm gonna merge this PR to get some of the quick CLI replacements in and then tackle the options again. In the end this will change this line above

finalize: userOptions.finalize ?? true,
validate: userOptions.validate ?? false,
vcsRemote: userOptions.vcsRemote ?? "origin",
Expand Down
3 changes: 2 additions & 1 deletion packages/bundler-plugin-core/src/sentry/cli.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { InternalOptions } from "../options-mapping";
import { Logger } from "./logger";

type SentryDryRunCLI = { releases: Omit<SentryCliReleases, "listDeploys" | "execute"> };
type SentryCLILike = SentryCli | SentryDryRunCLI;
export type SentryCLILike = SentryCli | SentryDryRunCLI;

/**
* Creates a new Sentry CLI instance.
Expand All @@ -22,6 +22,7 @@ export function getSentryCli(internalOptions: InternalOptions, logger: Logger):
});

if (internalOptions.dryRun) {
logger.info("In DRY RUN Mode");
return getDryRunCLI(cli, logger);
}

Expand Down
2 changes: 2 additions & 0 deletions packages/bundler-plugin-core/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { Hub } from "@sentry/hub";
import { Span } from "@sentry/tracing";
import { SentryCLILike } from "./sentry/cli";
import { createLogger } from "./sentry/logger";

/**
Expand Down Expand Up @@ -348,4 +349,5 @@ export type BuildContext = {
hub: Hub;
parentSpan?: Span;
logger: ReturnType<typeof createLogger>;
cli: SentryCLILike;
};
6 changes: 4 additions & 2 deletions packages/bundler-plugin-core/test/cli.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,11 @@ describe("getSentryCLI", () => {
expect(cli).toBeInstanceOf(SentryCli);
});

it("returns a valid CLI instance if dryRun is set to true", () => {
it("returns a dry run CLI stub if `dryRun` is set to true", () => {
const logger = { info: jest.fn() };
// eslint-disable-next-line @typescript-eslint/no-unsafe-argument, @typescript-eslint/no-explicit-any
const cli = getSentryCli({ dryRun: true } as any, {} as any);
const cli = getSentryCli({ dryRun: true } as any, logger as any);
expect(logger.info).toHaveBeenCalledWith(expect.stringMatching("DRY RUN"));
expect(cli).not.toBeInstanceOf(SentryCli);
});
});
85 changes: 0 additions & 85 deletions packages/bundler-plugin-core/test/getReleaseName.test.ts
View file
Open in desktop

This file was deleted.