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

fix driver detection #1209

Merged
merged 4 commits into from
Oct 10, 2022
Merged

fix driver detection #1209

merged 4 commits into from
Oct 10, 2022

Conversation

casperdcl
Copy link
Contributor

@casperdcl casperdcl commented Oct 7, 2022

@casperdcl casperdcl added bug Something isn't working technical-debt Refactoring, linting & tidying p0-critical Max priority (ASAP) labels Oct 7, 2022
@casperdcl casperdcl requested a review from 0x2b3bfa0 October 7, 2022 08:39
@casperdcl casperdcl self-assigned this Oct 7, 2022
@casperdcl casperdcl temporarily deployed to internal October 7, 2022 08:39 Inactive
@casperdcl casperdcl marked this pull request as ready for review October 7, 2022 08:39
@casperdcl casperdcl requested a review from a team October 7, 2022 08:39
@0x2b3bfa0
Copy link
Member

I was about to open a pull request with the same change yesterday, but... are you sure it fixes the issue? 🤔

@0x2b3bfa0
Copy link
Member

As per the original bug report, this is the error:

cml/src/cml.js

Line 153 in 4ec6517

if (!driver) throw new Error('driver not set');

@casperdcl
Copy link
Contributor Author

casperdcl commented Oct 7, 2022

afaik there's nothing wrong with that line - it's correctly raising an error. The real cause must be elsewhere?

@casperdcl casperdcl temporarily deployed to internal October 7, 2022 10:50 Inactive
@0x2b3bfa0
Copy link
Member

0x2b3bfa0 commented Oct 7, 2022

The real cause must be elsewhere?

Yes, sure! I still haven't found the GPS coordinates of elsewhere, though.

@0x2b3bfa0
Copy link
Member

Class constructor

cml/src/cml.js

Lines 117 to 118 in 9cfa5ab

class CML {
constructor(opts = {}) {

cml/src/cml.js

Line 128 in 9cfa5ab

this.driver = driver || inferDriver({ repo: this.repo });

Driver detection function

cml/src/cml.js

Lines 69 to 80 in 4ec6517

const inferDriver = (opts = {}) => {
const { repo } = opts;
if (repo) {
if (repo.includes('github.com')) return GITHUB;
if (repo.includes('gitlab.com')) return GITLAB;
if (/bitbucket\.(com|org)/.test(repo)) return BB;
}
if (GITHUB_REPOSITORY) return GITHUB;
if (CI_PROJECT_URL) return GITLAB;
if (BITBUCKET_REPO_UUID) return BB;
};

Thrown error

cml/src/cml.js

Lines 151 to 153 in 4ec6517

getDriver() {
const { driver, repo, token } = this;
if (!driver) throw new Error('driver not set');

@0x2b3bfa0
Copy link
Member

0x2b3bfa0 commented Oct 7, 2022

I assume that the class constructor will always be called, and the only case where this.driver can remain unset is when none of the inferDriver conditions are met, which seems pretty unlikely to me.

dacbd
dacbd previously approved these changes Oct 7, 2022
Copy link
Contributor

@dacbd dacbd left a comment

Choose a reason for hiding this comment

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

Perhaps a look at the things @0x2b3bfa0 mentioned is required, but these are clearly wrong, and we can fix them now.

@0x2b3bfa0
Copy link
Member

Yes, but #1209 (this) does not fix #1066 unless proven otherwise

@0x2b3bfa0 0x2b3bfa0 force-pushed the master branch 3 times, most recently from e2bed3a to fe9a06f Compare October 8, 2022 04:28
@dacbd dacbd temporarily deployed to internal October 8, 2022 22:29 Inactive
@dacbd
Copy link
Contributor

dacbd commented Oct 8, 2022

de-link #1066 and merge?

@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal October 10, 2022 03:18 Inactive
@0x2b3bfa0 0x2b3bfa0 enabled auto-merge (squash) October 10, 2022 03:19
0x2b3bfa0
0x2b3bfa0 previously approved these changes Oct 10, 2022
Copy link
Member

@0x2b3bfa0 0x2b3bfa0 left a comment

Choose a reason for hiding this comment

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

Fine, but doesn't fix #1066

@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal October 10, 2022 03:24 Inactive
@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal October 10, 2022 03:29 Inactive
@0x2b3bfa0 0x2b3bfa0 merged commit e9546a2 into master Oct 10, 2022
@0x2b3bfa0 0x2b3bfa0 deleted the fix-driver branch October 10, 2022 03:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working p0-critical Max priority (ASAP) technical-debt Refactoring, linting & tidying
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants