-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat: Integrating the web dashboard feature #3505
Merged
Merged
Changes from 28 commits
Commits
Show all changes
30 commits
Select commit
Hold shift + click to select a range
31569f1
feat: Integrating the web dashboard feature
szkiba 099fdae
fix: linter issue
szkiba ad5599b
upgrading xk6-dashboard to v0.7.0-alpha.1 pre-release
szkiba 43f2990
Merge remote-tracking branch 'origin/master' into feat/3504-dashboard…
szkiba 7ac6f77
the web dashboard is now inactive by default
szkiba 8eaa500
make more consistent with other outputs
szkiba 66c7e57
simplify web-dashboard ui output
szkiba 5ec840a
upgrade xk6-dashboard to v0.7.0-apha.2
szkiba 4764e9e
increased indent by 1 (due to "web-dashboard" length)
szkiba bf371b1
test: added web dashboard CLI and env test
szkiba 7c7c3b8
Merge remote-tracking branch 'origin/master' into feat/3504-dashboard…
szkiba 82d9942
log failed web dashboard test (locally runs without fail)
szkiba d5f0b54
test: log web dashboard test run to stdout
szkiba aec3854
Merge remote-tracking branch 'origin/master' into feat/3504-dashboard…
szkiba 303ce2c
fix: Excluded xk6-dashboard config.json from .gitignore
szkiba d0b0ed3
web-dashboard output name renamed to internal-web-dashboard
szkiba 4548414
move dashboard config.json line close to the rule it is overwriting
szkiba cf63408
upgrade xk6-dashboard to v0.7.0-alpha.4
szkiba c2b20c3
use xk6-dashboard output name from xk6-dashboard source code
szkiba 79d55f6
document excluding ule
szkiba a9f97d0
upgrade xk6-dashboard dependency to v0.7.0-alpha.5
szkiba dfa5a67
hide web-dashboard flag
szkiba 06b47cb
Update .gitignore
oleiade fe38865
Merge remote-tracking branch 'origin/master' into feat/3504-dashboard…
szkiba 99e5ac1
fix linter issue
szkiba 233c05a
upgrade xk6-dashboard to v0.7.0-alpha.6
szkiba ee587cb
upgraded xk6-dashboard to v0.7.0 release
szkiba ef727f6
remove web-dashboard flag
szkiba eb67aef
test: fix web-dashboard flag test
szkiba 7f43e50
upgrade xk6-dashboard to v0.7.1 to fix abort issue
szkiba File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this and the changes to the way we format stuff ... I wonder ... why?
Like why do you want ot have an additional
--web-dashboard
cli flag when-o dashboard
... works.Why do you want to move it on separate line when outputs already have a way to show a url for users to use as in
^ arguably the above can be improved, but I see no reason to move it in a different one.
And this will practically limit the integration to the above "registration" of the output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this is what is written in the design document, if you want to re-discuss it open the question to the working group. It's perfectly fine for me if it has to be activated with the -o flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @szkiba mentioned, this was a UX/product decision made by the workgroup that is reflected in the design document.
Part of the reasoning behind that decision was a desire to have the dashboard become a first-class citizen of the k6 UX down the road, the fact that it acts as an output under the hood being judged an implementation detail.
If there is a strong technical argument to be made to keep exposing/displaying it as an output as part of our command-line UI, we'd be happy to discuss it at the beginning of next year when everyone is back 🤝
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Outputs are first class citizen.
Kind of implies to me this shouldn't be in the initial release.
The technical and none technical argument is the same - users already have the concept of output. Adding a thing that is clearly and output and then treating it differently isn't cohesive.
At best people will be confused why there are two ways to the same thing at worse they will think they are two different things. And anyone who works on it will need to wonder the same thing.
On connected note - yes the current summary fits this same bill, except that it predates most of that, and if possible I will certainly try to push it towards being an output through and through.
But adding more special cases IMO doesn't help anyone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point 👍🏻 I personally don't have a strong opinion on this.
Will add it to the agenda of our internal discussions when everyone is back from 🎄 holidays, and I'll invite you to it so we can discuss and decide on a way forward that makes sense for everyone 🙇🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've discussed this internally, and our preference goes with proceeding with the current UX provided by the PR:
--output
argument is supported for consistency and to align with a larger idea to mark outputs (including the end-of-test summary), more explicitly as such in the future.