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.

Sign up for GitHub

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

Jump to bottom

Make using v8flags package optional #2643

Closed
wants to merge 1 commit into from
Closed

Make using v8flags package optional #2643

wants to merge 1 commit into from

Conversation

GabrielCastro
Copy link

@GabrielCastro GabrielCastro commented Jun 3, 2018 •
edited

The v8flags package requires a file to be opened in read/write mode,
we want to be able to run knex cli migrate inside a contains with a read-only file system

@kibertoad
Copy link
Collaborator

@GabrielCastro Crash on node 8 is probably random, rerunning tests should resolve it.
Is it possible to add tests for the change?

@GabrielCastro
Copy link
Author

I can't seem to find existing tests for the cli, could you point me in the right direction.

@kibertoad
Copy link
Collaborator

@GabrielCastro Doesn't look like any exist at this point; you don't have to cover all of the functionality with tests, just the new stuff should be ok :).

@kibertoad
Copy link
Collaborator

@GabrielCastro You can take a look at #2884 for CLI tests, we finally have an example of how to do it!

@kibertoad
Copy link
Collaborator

@GabrielCastro Status update :)?

@markmsmith
Copy link

I'm also interested in this fix. The not-so-great workaround was to define the environment variable TMPDIR=/dev/shm

@kibertoad
Copy link
Collaborator

@markmsmith Would you consider rebasing the PR?

@markmsmith
Copy link

I'm currently talking with my company's legal to department to get approval to contribute (if the original author doesn't come back). Do you have any kind of CLA that needs signed?

@kibertoad
Copy link
Collaborator

Nope, we don't have anything like that.

@kibertoad
Copy link
Collaborator

@GabrielCastro This still needs to be documented. Could you please add documentation PR to https://github.com/knex/documentation?

kibertoad
kibertoad reviewed Aug 24, 2019
View reviewed changes
bin/cli.js
v8flags = require('v8flags');
}

var cli = new Liftoff({
Copy link
Collaborator

Choose a reason for hiding this comment

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

can be const

kibertoad
kibertoad reviewed Aug 24, 2019
View reviewed changes
bin/cli.js
name: 'knex',
extensions: interpret.jsVariants,
v8flags: require('v8flags'),
v8flags: v8flags,
Copy link
Collaborator

Choose a reason for hiding this comment

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

can be just v8flags

@kibertoad
Copy link
Collaborator

@markmsmith Ping?..

@kibertoad
Copy link
Collaborator

v8flags was removed, so this is no longer relevant

@kibertoad kibertoad closed this Dec 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kibertoad kibertoad kibertoad left review comments

Assignees
No one assigned
Labels
help wanted missing documentation missing tests needs changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@GabrielCastro @kibertoad @markmsmith