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

Setup optional telemetry authorization #446

Merged
merged 4 commits into from
Nov 28, 2023
Merged

Setup optional telemetry authorization #446

merged 4 commits into from
Nov 28, 2023

Conversation

quesabe
Copy link
Collaborator

@quesabe quesabe commented Nov 23, 2023

Closes PLA-277.

If telemetry endpoint requires authorisation, it can be provided in form of a header. Two additional options are defined:

  • header name,
  • GitHub secret name to pull the token from.

@quesabe quesabe self-assigned this Nov 23, 2023
Copy link

linear bot commented Nov 23, 2023

PLA-277

const authHeaderName = project.telemetryAuthHeader

if (authHeaderName) {
headers[authHeaderName] = process.env[telemetryAuthToken]!
Copy link
Contributor

Choose a reason for hiding this comment

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

@quesabe would this make naming more consistent?

Suggested change
headers[authHeaderName] = process.env[telemetryAuthToken]!
headers[reportTargetAuthHeaderName] = process.env[reportTargetAuthToken]!

Also let's probably rename project.telemetryReportUrl to project.reportTargetUrl.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@gvidon Updated the option names.

BTW, since we added more options shouldn't we move them into an object instead of residing them all on the root of the options. E.g.:

export interface TelemetryOptions {
  /**
   * Endpoint URL to send telemetry data to.
   */
  readonly reportTargetUrl?: string

  /**
   * Authorization header name for telemetry
   */
  readonly reportTargetAuthHeaderName?: string

  /**
   * The name of env var to extract header value from.
   * The value is expected to be stored in a CI secret.
   */
  readonly reportTargetAuthTokenVar?: string
}

export interface WithTelemetry {
  /**
   * Enable template usage telemetry.
   * Collects the data on the template package version, connected git remotes,
   * applied escape hatches, configured GitHub workflows.
   *
   * @default false
   */
  readonly isTelemetryEnabled?: boolean

  /**
   * Configuration options for telemetry
   */
  readonly telemetryOptions?: TelemetryOptions
}

Copy link
Contributor

Choose a reason for hiding this comment

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

@quesabe yeah, an object is a good idea. Should we add it per this PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see it's merged. Let me create a separate one.

src/common/telemetry/setup-telemetry.ts Outdated Show resolved Hide resolved
quesabe and others added 3 commits November 26, 2023 20:28
Disable TelemetryWorkflow generation for subprojects. Throw an error on unexisting ProjenrcFile component (again valid for subprojects that rely on root project ProjenrcFile).
}

/**
* Creates a workflow within the given project
* or within the project parent (for subprojects).
*/
static addToProject(project: NodeProject, options: TelemetryWorkflowOptions) {
const {outdir, workflowNodeVersion} = options
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also added a commit that removes any logic related to parent-subproject relation, since we aim for projenrc file that is only present on root project in a nested setup.

@quesabe quesabe requested a review from gvidon November 26, 2023 18:22
@gvidon gvidon merged commit 564c34d into main Nov 28, 2023
6 checks passed
@gvidon gvidon deleted the telemetry-auth branch November 28, 2023 00:03
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.

2 participants