Skip to content

feat(config): Automatically detect GitHub config when missing - #208

Merged
BYK merged 11 commits into
masterfrom
byk/feat/auto-github-config
May 5, 2021
Merged

BYK merged 11 commits into
masterfrom
byk/feat/auto-github-config

Conversation

@BYK

@BYK BYK commented Apr 28, 2021

Copy link
Copy Markdown
Member

We don't need people to explicitly state github global configuration
as it can be detected from git itself. Since we rely on the presence
of a git repo in publish and prepare we can safely rely on this
too and remove another piece of redundant configuration.

Most of the changes in this patch are related to the conversion of getGlobalGithubConfig from a sync function to an async function. There were also a few places which relied on config.github directly which is also removed now.

Ideally we'd abstract the raw config object in the future and provide a global, normalized configuration to all downstream dependent code.

This patch also adds tests around this new functionality.

@BYK
BYK requested a review from chadwhitacre April 28, 2021 18:59
@BYK
BYK force-pushed the byk/feat/auto-github-config branch from 4de0ecd to 7563748 Compare April 29, 2021 08:19
We don't need people to explicitly state `github` global configuration
as it can be detected from `git` itself. Since we rely on the presence
of a `git` repo in `publish` and `prepare` we can safely rely on this
too and remove another piece of redundant configuration.

This patch also adds tests around this.
@BYK
BYK force-pushed the byk/feat/auto-github-config branch from fa5eb63 to 4966757 Compare April 30, 2021 19:22
@BYK
BYK marked this pull request as ready for review May 4, 2021 11:20
@BYK
BYK requested review from jan-auer and tonyo May 4, 2021 11:20
@BYK

BYK commented May 4, 2021

Copy link
Copy Markdown
Member Author

I will add the tests for this new functionality separately as they cause some very hard to debug import errors. I'm guessing it has something to do with Jest and mocking but I've already spent multiple full days on debugging without any luck and now this PR blocks #209 (due to its github config refactor).

Comment thread src/__tests__/config.test.ts
Comment thread src/commands/prepare.ts
Comment thread src/config.ts
Comment thread package.json Outdated
Comment thread src/config.ts
Comment thread src/targets/base.ts
config: TargetConfig,
artifactProvider: BaseArtifactProvider
artifactProvider: BaseArtifactProvider,
githubRepo: GithubGlobalConfig

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we default this to null/undefined to avoid having to modify every target, since most targets don't need this?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm not a fan of that as then we need to do non-null checks down the line. This will especially be pronounced in publish where we get all the targets without knowing the exact target type (as it is determined at runtime).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we? Why would we need to check for null if the only targets that use githubRepo are the ones that will actually have it in the constructor?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Even if they have it in the constructor, the type system will force us to make sure it is not null. We may be able to overcome this so I'll look into that later.

@BYK
BYK enabled auto-merge (squash) May 5, 2021 07:47
@BYK
BYK disabled auto-merge May 5, 2021 07:47
@BYK
BYK enabled auto-merge (squash) May 5, 2021 07:48
@BYK
BYK merged commit beb6c4d into master May 5, 2021
@BYK
BYK deleted the byk/feat/auto-github-config branch May 5, 2021 14:20
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