feat: ingestion #64
feat: ingestion #64
Conversation
marco-ippolito
force-pushed
the
ingestion
branch
from
787ecc3
to
93d867d
Compare
|
Apparently the errors are caused by Apollo client not being supported in Node v14 and v16 |
marco-ippolito
force-pushed
the
ingestion
branch
2 times, most recently
from
0a56671
to
c4e592c
Compare
I'd add that as next step and pass it as argument like |
|
|
||
| ```bash | ||
| # Send information about your project manifest files | ||
| NES_REPORT_URL="https://example.com/graphql" hdcli report generate |
There was a problem hiding this comment.
This syntax does not work acorss all OSes (e.g. on Windows). Do we care about showing instructions that might not work for many users in the README?
(In general, I am a bigger fan of expliit options rather than environment variables, since (a) they tend to behave more consistent across systems, (b) the make the commands more self-contained (and avoid a form of "spookie action at a distance" kind of behavior) and (c) it is easy to turn an env var into a cli arg (--some-arg=$MY_ENV_VAR), while the opposite is not possible 😁
Just my two cents on the subject 🪙
There was a problem hiding this comment.
I mean this is just for development reasons, why a customer would ever want to change the endpoint?
Agree its not cross platform, we could change it to use crossenv, but I would not change it to a flag
| | JSONValue[]; | ||
|
|
||
| export type Options = { | ||
| all: boolean; |
There was a problem hiding this comment.
Do we really use the all option?
There was a problem hiding this comment.
Nope I should drop it 😁 copy/paste error
| @@ -0,0 +1,39 @@ | |||
| import { type ArgumentsCamelCase, type CommandModule } from 'yargs'; | |||
| import { askConsent, promptClientName, promptToProceedUploadFile } from './prompts'; | |||
There was a problem hiding this comment.
promptToProceedUploadFile() doesn't seem to be used 😱
There was a problem hiding this comment.
It is used in the sendManifest which is in another file 🤔 formatter should have removed it
| aliases: ['gen', 'g'], | ||
| builder: { | ||
| consent: { | ||
| describe: 'I understand that sensitive data may be uploaded to the server', |
There was a problem hiding this comment.
I think there was a misunderstanding in #64 (comment). This is not a message to be shown to the user when prompting them to consent. This is the description of the CLI --consent option, which the user can pass to the command to avoid being prompted to give their consent.
So, I think that something along the lines of Agree to understanding that sensitive data may be sent to the server (similar to the diagnostics one is more appropriate.
| }; | ||
|
|
||
| async function run(args: ArgumentsCamelCase<Options>): Promise<void> { | ||
| const consent = await askConsent(args); |
There was a problem hiding this comment.
FWIW, I find the current process wrt consent somewhat confusing and inconsistent.
Why do we need to have two consent prompts and also why only suppress the one of them if the --consent option is passed?
I suggest the following:
- Look for the manifest file (without concerning ourselves with consent first).
- If they have passed the
--consentoption, upload the data. - If the have not passed the
--consentoption, prompt them for consent to upload the specific file we have found (so that they have a chance to review it if needed). - If they give consent, upload the data.
flowchart TB
A[Start] --> B
B[Find manifest file] --> C
C{'--consent' option?} -- Yes --> D
D[Upload data] --> E[End]
C -- No --> F
F[Prompt for consent] --> G
G{Given consent?} -- Yes --> D
G -- No --> E
There was a problem hiding this comment.
It's not about asking consent twice, is that in case of multiple manifest found the user can choose which one to send , and avoid sending the wrong one by mistake. I could immagine in a php project there might be pckage.json or composer.json. So asking the user to confirm he wants to send that specific package makes sense to me.
marco-ippolito
force-pushed
the
ingestion
branch
3 times, most recently
from
9839f5b
to
4d48bb8
Compare
marco-ippolito
force-pushed
the
ingestion
branch
from
4d48bb8
to
4a88385
Compare
This features allows customer to send their package.json.
Bikeshedding or error messages, prompts, naming welcome 😄