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

chore: add a utility script to make development easier #512

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bruno-fs
Copy link
Contributor

@bruno-fs bruno-fs commented Nov 6, 2024

npm run start:using-server will serve the development UI properly wired to a running instance of quipucords server. Default configuration points to quipucords-installer defaults, and can be customized with QUIPUCORDS_SERVER_* variables.

npm run start:using-server will serve the development UI properly
wired to a running instance of quipucords server. Default configuration
points to quipucords-installer defaults, and can be customized
with QUIPUCORDS_SERVER_* variables.
@bruno-fs bruno-fs requested a review from a team November 7, 2024 12:33
Copy link
Member

@cdcabrera cdcabrera left a comment

Choose a reason for hiding this comment

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

just a couple of choices, and a non-blocking recommendation

@@ -28,6 +28,7 @@
"build:pre": "bash ./scripts/pre.sh",
"release": "changelog --link-url https://github.com/quipucords/quipucords-ui.git",
"start": "export PROTOCOL=http; export HOST=127.0.0.1; export PORT=${PORT:-3000}; export MOCK_PORT=${MOCK_PORT:-3030}; run-p -l api:dev start:js start:open",
"start:using-server": "export PROTOCOL=${QUIPUCORDS_SERVER_PROTOCOL:-https}; export HOST=${QUIPUCORDS_SERVER_HOST:-127.0.0.1}; export PORT=${PORT:-3000}; export MOCK_PORT=${QUIPUCORDS_SERVER_PORT:-9443}; run-p -l start:js start:open",
Copy link
Member

@cdcabrera cdcabrera Nov 7, 2024

Choose a reason for hiding this comment

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

some level of alteration is needed, a couple of choices for you...

Option 1: highly recommend

  • combining the original and new script. reason, currently, the new script is customized for a single developer where as the original start script was meant to be expandable for multiple use cases, modifying the original would be natural

    technically the npm-run-all is just looking for strings to run npm scripts so replacing it with an exported empty command means it skips it...

       "start": "export PROTOCOL=${DEV_SERVER_PROTOCOL:-http}; export HOST=${DEV_SERVER_HOST:-127.0.0.1}; export PORT=${DEV_SERVER_PORT:-3000}; export MOCK_PORT=${DEV_SERVER_MOCK_PORT:-3030}; export SERVER_SCRIPT=${DEV_SERVER_SCRIPT:-api:dev}; run-p -l $SERVER_SCRIPT start:js start:open",
    

Option 2: if the unique script is kept...

  • start:using-server should be renamed...
    • reason, description is generic. technically the original script is also running various servers
    • recommend something like start:custom-server or something direct like start:custom since having to continuously type something with a hyphen gets tedious, for those who still reboot/logout their computers =) (there are regrets around the integration-dev script naming)

And an additional recommendation, but not necessary

  • the prefix QUIPUCORDS_SERVER_ should probably be shortened
    • reason, if the values using QUIPUCORDS_SERVER_ are expected to be typed out by follow up developers we could be kind by shorten them. But, if there's another script exporting these values this is less of a concern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try to work with option 1! Better not to have unecessary extra commands.

now, about QUIPUCORDS_ prefixes being long, this is a team decision. We want ALL env vars configuring quipucords to have this prefix, even if it makes them longer.

@cdcabrera
Copy link
Member

Another recommendation is to update the commit from chore to build since you're actively affecting the scripts that let us run and build everything

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants