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

lint: fix camelCase #554

Merged
merged 5 commits into from
May 27, 2021
Merged

lint: fix camelCase #554

merged 5 commits into from
May 27, 2021

Conversation

casperdcl
Copy link
Contributor

@casperdcl casperdcl commented May 21, 2021

fixes #547

  • The first commit is programmatically generated.
    • NB: don't squash-merge (to keep bot commit separate)
  • deDe-camel
  • The actual commits to check are https://github.com/iterative/cml/compare/7741a6f...6b28b84?w=1. In particular, my handling of:
    • rm-watermark
    • commit-sha
    • mime-type
    • head-sha
    • user-name
    • user-email
    • gitlab-uploads
    • startup-script
    • ssh-private
    • spot-price
    • idle-timeout
    • hdd-size
    • cloud-ssh-private-visible
    • pipe-args
    • git-url-parse
    • tf-resource

which is annoying due to yargs/yargs#1679

Snake is dead. Snake? Snake! SNAKE!

@casperdcl casperdcl added technical-debt Refactoring, linting & tidying testing Unit tests & debugging labels May 21, 2021
@casperdcl casperdcl self-assigned this May 21, 2021
@casperdcl casperdcl temporarily deployed to test-internal May 21, 2021 08:27 Inactive
@casperdcl casperdcl marked this pull request as draft May 21, 2021 08:29
@casperdcl casperdcl temporarily deployed to test-internal May 21, 2021 10:59 Inactive
@casperdcl casperdcl temporarily deployed to test-internal May 21, 2021 11:32 Inactive
@casperdcl casperdcl marked this pull request as ready for review May 21, 2021 11:43
bin/cml-runner.js Outdated Show resolved Hide resolved
@0x2b3bfa0

This comment has been minimized.

@casperdcl casperdcl temporarily deployed to test-internal May 21, 2021 15:06 Inactive
Copy link
Contributor

@DavidGOrtega DavidGOrtega left a comment

Choose a reason for hiding this comment

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

lets try /tests first

bin/cml-runner.js Outdated Show resolved Hide resolved
src/cml.test.js Show resolved Hide resolved
@casperdcl casperdcl temporarily deployed to test-internal May 21, 2021 15:16 Inactive
@casperdcl casperdcl temporarily deployed to test-internal May 21, 2021 15:17 Inactive
@casperdcl
Copy link
Contributor Author

Where's this --tf_resource flag from? https://gitlab.com/iterative.ai/cml/-/jobs/1283674644#L7031

@0x2b3bfa0
Copy link
Member

0x2b3bfa0 commented May 21, 2021

Where's this --tf_resource flag from?

You've discovered the arcanes of the Terraform Provider 🧙🏼‍♂️

@0x2b3bfa0
Copy link
Member

The provider is tightly coupled to CML, so we need to preserve backwards compatibility with the old underscore option for now.

@casperdcl casperdcl temporarily deployed to test-internal May 21, 2021 15:43 Inactive
@casperdcl casperdcl temporarily deployed to test-internal May 21, 2021 15:44 Inactive
@casperdcl casperdcl temporarily deployed to test-internal May 21, 2021 15:45 Inactive
casperdcl and others added 3 commits May 24, 2021 23:28
Partially reverts commit eca3a98
Co-authored-by: Helio Machado <0x2b3bfa0+git@googlemail.com>
@casperdcl casperdcl temporarily deployed to test-internal May 24, 2021 22:28 Inactive
@casperdcl casperdcl temporarily deployed to test-internal May 25, 2021 09:26 Inactive
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.

Looks good to me, including fc5a0ab!

@casperdcl
Copy link
Contributor Author

/tests

src/cml.js Show resolved Hide resolved
@DavidGOrtega
Copy link
Contributor

DavidGOrtega commented May 25, 2021

@0x2b3bfa0

Looks good to me, including fc5a0ab!

why fc5a0ab looks good?
It has saved a decent amount of headaches in the past.
A common GITHUB_TOKEN can access the api but not the workflows api. If the token is not set it CML will tell first and if the token is set most probable error, having into account that GITHUB_TOKEN is the default token that is probably set, is that the token has no rights to do that. If not set that way is the error descriptive enough for us just to know or at least to hint it?

@0x2b3bfa0
Copy link
Member

The issue with the current approach is that we're outputting the following message no matter what the actual error is:

Error: REPO_TOKEN does not have enough permissions to access workflow API

Removing the catch-all block, we would get instead this slightly more opaque message in the case you describe:

RequestError [HttpError]: Resource not accessible by integration

It's not as explicit as the previous message, but at leaset it won't mask unrelated errors behind the same description.

@casperdcl
Copy link
Contributor Author

casperdcl commented May 25, 2021

Ah FYI I don't intend to include fc5a0ab in this PR (will revert and leave for another issue) - it was a debug commit and clearly failed as I later realised that the / tests command incorrectly uses an NPM-release rather than the current state of the code so completely ignores fc5a0ab anyway. Which is yet another issue to be addressed elsewhere.

In any case the / tests command appears to be commented as:

# check that CML container builds properly

in which case it is successful (the docker image does indeed build successfully, and the later failure is unrelated to this PR).

I propose to merge without fc5a0ab. Any objections?

@casperdcl casperdcl temporarily deployed to test-internal May 25, 2021 18:38 Inactive
@casperdcl casperdcl changed the base branch from master to devel May 25, 2021 18:40
@DavidGOrtega
Copy link
Contributor

DavidGOrtega commented May 25, 2021

The issue with the current approach is that we're outputting the following message no matter what the actual error is:

As I say the most probable error at that point specially in the CI is that. The CI is going to provide always a GITHUB_TOKEN.

We could make the question or suggest the error like many others do to hint.

This normally means .... or Does your token have workflow permissions?

But as I say its going to be very rare that the error is not that. It is even always telling you the truth.
If you miss to setup a proper token its correctly telling you that the token, even incorrect one, has no permissions to do such thing

Copy link
Contributor

@DavidGOrtega DavidGOrtega left a comment

Choose a reason for hiding this comment

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

Caan we please squash and merge? 🙏

@casperdcl
Copy link
Contributor Author

@DavidGOrtega I've removed that debug commit (we should leave it for another issue) - have you reviewed and are you happy with this PR as-is?

Copy link
Contributor

@DavidGOrtega DavidGOrtega left a comment

Choose a reason for hiding this comment

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

tf-resource what happened to it?

Copy link
Contributor

@DavidGOrtega DavidGOrtega left a comment

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
technical-debt Refactoring, linting & tidying testing Unit tests & debugging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lint: fix issues
4 participants