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

set base URL for GHES #449

Merged
merged 1 commit into from May 7, 2020

Conversation

ericsciple
Copy link
Contributor

@ericsciple ericsciple commented May 6, 2020

Related to ADR for setting GHE URLs (in c2c-actions)

The toolkit should use the GITHUB_API_URL and GITHUB_GRAPHQL_URL to connect to GHES or Dotcom.

@ericsciple ericsciple marked this pull request as ready for review May 6, 2020 20:34
@@ -61,14 +61,19 @@ export class GitHub extends Octokit {
const token = args[0]
const options = {...args[1]} // Shallow clone - don't mutate the object provided by the caller

// Base URL - GHES or Dotcom
Copy link
Contributor Author

@ericsciple ericsciple May 6, 2020

Choose a reason for hiding this comment

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

I'll look before merging whether any unit test can be added. Might be able to verify the expected baseUrl is set and whether proxy is set based on the correct URL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unfortunately doesnt appear to be a way to get the options after it's constructed

@ericsciple
Copy link
Contributor Author

notes from offline review:

  • bump version, add release notes

Copy link
Collaborator

@thboop thboop left a comment

Choose a reason for hiding this comment

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

LGTM, would be nice if we can add unit tests

process.env['GITHUB_GRAPHQL_URL'] || 'https://api.github.com/graphql'

// Remove trailing "/graphql"
if (url.toUpperCase().endsWith('/GRAPHQL')) {
Copy link
Member

Choose a reason for hiding this comment

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

There's no possibility of a trailing slash is there? Alternative https://stackoverflow.com/a/16750711/775184

Copy link
Contributor Author

Choose a reason for hiding this comment

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

shouldnt be a trailing slash, i'll trim on launch side when setting the urls

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also not bad to add resiliency here, i added code to trim trailing slash if detected

@ericsciple
Copy link
Contributor Author

Going ahead and merging since failure due to audit service being down. Retried twice, still failed. Passed in a previous run and not changing dependencies with this PR.

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.

None yet

3 participants