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.
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
feat: Integrating the web dashboard feature #3505
Changes from 27 commits
31569f1
099fdae
ad5599b
43f2990
7ac6f77
8eaa500
66c7e57
5ec840a
4764e9e
bf371b1
7c7c3b8
82d9942
d5f0b54
aec3854
303ce2c
d0b0ed3
4548414
cf63408
c2b20c3
79d55f6
a9f97d0
dfa5a67
06b47cb
fe38865
99e5ac1
233c05a
ee587cb
ef727f6
eb67aef
7f43e50
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.