Audit application installations #27
Conversation
Codecov Report
@@ Coverage Diff @@
## main #27 +/- ##
===========================================
+ Coverage 26.04% 36.53% +10.49%
===========================================
Files 4 4
Lines 96 104 +8
Branches 13 13
===========================================
+ Hits 25 38 +13
+ Misses 71 66 -5
Continue to review full report at Codecov.
|
| ``` | ||
|
|
||
| Both of the above are wholly equivalent; it's simply a matter of preference | ||
| which way you would like to specify them. |
There was a problem hiding this comment.
for folks that might not be so intimately familiar with toml, I'd recommend giving an example of how you might define multiple applications
There was a problem hiding this comment.
Hmmm, that's a good call. I do have a link to the TOML documentation which does have examples, but it's likely people won't click through. I also want to avoid making this document into a TOML tutorial, or too long. I think I'll add a second application to the example.toml.
| appId = 42 | ||
| appSlug = "infinite-improbability-drive" | ||
| repositorySelection = "selected" |
There was a problem hiding this comment.
as a consumer of this library, where do I find the values to use for these?
There was a problem hiding this comment.
Hmmm yeah good question. The app id and slug I was only able to find after actually querying our org. I honestly don't know of a better way to find them, so that may have to be our recommendation for now: run the tool and plug the data into your config. Which gives me a great idea for a future feature: orglinter --generate-config It could query the org and produce a valid config file based on its current state.
Oh as for the repositorySelection I can document that "selected" and "all" are the only values. I think. I haven't been able to find solid documentation on that either. The REST API docs are terrible!
| @@ -94,12 +95,39 @@ async function retrieveOrgInfo(orgName, token) { | |||
| // eslint-disable-next-line id-length | |||
| requiresTwoFactorAuthentication: organization.requiresTwoFactorAuthentication, | |||
| twitterUsername: organization.twitterUsername, | |||
| websiteUrl: organization.websiteUrl | |||
| websiteUrl: organization.websiteUrl, | |||
| applications: await retrieveOrgApplications(orgName, token) | |||
There was a problem hiding this comment.
IMHO don't do inline awaits like this in object construction.
Also, given #26, you might start to think about how the different rulesets will need to define which information they'll need to function, how to make sure you're only running each data-fetch once, and how the data gets provided to the different rules.
There was a problem hiding this comment.
Why not an inline await? Is that more of a preference/style thing, or for technical reasons?
As far as #26 though yeah I should probably make this line applications: [], and move this out to a separate later line of result.applications = await .... Then later there can be something like if (config.checkApplications) result.applications = await ....
There was a problem hiding this comment.
Why not an inline await
There's not a technical reason, it works, but it can be hard to debug and hard to reason about imagine code like:
const stuff = {
foo: await foo(),
bar: await bar(),
fizzyLiftingDrinks: fizzyLiftingDrinks(),
fizzbuzz: await fizzbuzz()
};it can be hard to debug if there are issues. its hard to reason about the ordering of the promises and from looking at it it looks at first glance that it might be run in parallel, but that wouldn't be the case. Is the lack of await on the fizzyLiftingDrinks intentional or a bug?
Pulling the awaits out make it easier to reason about and the order more apparent and you could force it to be parallel if you wanted.
| before(function () { | ||
| sinon.stub(console, 'log'); | ||
| }); | ||
|
|
||
| after(function () { | ||
| sinon.restore(); | ||
| }); |
There was a problem hiding this comment.
IMHO make these be beforeEach and afterEach that way you're not leaking state between tests via sinon.
before/after - before/after the scope they exist in (in this case before & after everything within the describe, i.e. these will run once).
beforeEach/afterEach - before/after each test in the scope (i.e. these will run once per test within this describe)
There was a problem hiding this comment.
Hmm yeah probably a good call. My thought here was mostly to just stop console logs from happening and since I am not actually checking these logs I only wanted just a single call as opposed to many. But I could see wanting to check the log values later on.
| describe('retrieveOrgApplications', function () { | ||
| let scope; | ||
|
|
||
| before(async function () { |
There was a problem hiding this comment.
same with these, make it beforeEach/afterEach
|
Kudos, SonarCloud Quality Gate passed!
|
Fixes #23
Not only is this the first venture into using the REST API for a loader... it's also my first venture into mocking/testing an http(s) call! Hooray!
Implemented so far:
example.toml