-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
UI: add exec terminal #6697
UI: add exec terminal #6697
Conversation
This is adapted from helpful posts in this thread: ember-cli/ember-cli#2508 Without this, the origin header wasn’t being rewritten so I was unable to open a websocket to the API running on a different port.
The communication between the socket and xterm.js terminal instance might better be extracted, I’m not sure whether the component integration test even makes sense. I originally tried an acceptance test, but that seemed worse.
Hard-coding the command as |
Understood, @henrikjohansen, this is in very early stages and we plan to make it configurable, thanks. |
# Conflicts: # ui/yarn.lock
This seems quite fiddly, hopefully not Too Much™ 😳
It’s not that much of a navbar even… just not sure about how much sharing is even worth it here 🤔
This approach… not sure about it: • how to test the contents of the terminal without storing on window? • passing the terminal in seems a bit weird but otherwise was considering some event interface, but that seems like overkill…?!?
This isn’t responsive to resize events etc yet but at least looks better.
This seems a bit strange but…? Maybe a component would make sense to wrap everything in but then would it be rerendering as you navigate deeper into the hierarchy…? Maybe there’s some breakage if you navigate between task groups and something closes that you opened yourself but since I don’t anticipate actually changing the route when you click a task group (vs a task, which will), it seems safe to ignore.
I haven’t started to include allocations in all of this yet so this is pretty unrealistic.
This is becoming quite sprawling but I believe having good acceptance tests will help me clean up when I’ve covered the myriad approaches.
</div> | ||
<div> | ||
{{#if model.isRunning}} | ||
<div class="two-step-button"> |
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 considered having a duplication of this class called something like .like-two-step-button
because this isn’t an actual two-step button but using this class made it align properly with the other buttons in this section. It seemed maybe unnecessary…?
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.
Yeah, it's fine for now. I think the entire way we handle buttons in titles needs a larger rethink.
This is ready for review! I’ve extracted the nice-to-haves from that list above into issues and added links to them to the list. |
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.
This looks really great! Definitely good enough for the beta.
I had a few little things scattered throughout, but the only big thing that jumped out is the mock server. I'm not entirely sure what it's doing or why it was added.
But we can chat about that later, this is good to be merged now 😄
@@ -5,6 +5,5 @@ | |||
|
|||
Setting `disableAnalytics` to true will prevent any data from being sent. | |||
*/ | |||
"disableAnalytics": false, | |||
"proxy": "http://127.0.0.1:4646" | |||
"disableAnalytics": false |
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.
Is this to prevent the errors and socket pool saturation when nomad isn't running in the background? I have also ran into that, but I dunno if I'm ready to remove the proxy from here.
Thoughts on this?
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.
eep sorry, I meant to explain this. Just as I was starting to prototype this, I found that I was completely unable to connect to a websocket through that proxy because the Origin
didn’t match and the request was rejected by the API. The Javascript socket interface doesn’t give you any control over that for security reasons, so I switched to the technique described here because it gives you fine-grained control. The crux for this feature is this section where the origin gets rewritten in the proxy outside the browser. It’s a shame to lose this simplicity but it seems unavoidable :////////
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.
Wow. TIL. Definitely a shame when things don't just work, but given the amount of oddities in our use of HTTP, I'm surprised the default proxy even got us this far.
return activeStateTaskNames; | ||
}, | ||
[] | ||
); |
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 what you mean now about how hairy this traversal gets.
It doesn't make it any better, and in fact I think this is technically (but insignificant at this scale), but this approach with more method chaining might be more readable?
const activeStateTaskNames = this.taskGroup.allocations
.mapBy('states')
.reduce((allStates, states) => allStates.concat(states), [])
.filterBy('isActive')
.filter(taskState => taskState.task.taskGroup.name === this.taskGroup.name)
.mapBy('name');
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.
yeah, I conceptually like reduce but I agree that this is Too Much. I tried replacing with your suggestion and found it was rejecting everything, I don’t understand it:
this.taskGroup.allocations
.mapBy('states')
.reduce((allStates, states) => allStates.concat(states), [])
.mapBy('name')
// ["states", "states", "states", "states", "states", "states"]
this.taskGroup.allocations
.mapBy('states')
.reduce((allStates, states) => allStates.concat(states), [])
.filterBy('isActive')
// [undefined, undefined, undefined, undefined, undefined, undefined]
I don’t know what’s different about it but the collected and flattened objects don’t seem like the true task states…? I would like to do something like what you suggested though, so I opened this issue to help remember, thanks.
this.terminal.writeln(''); | ||
} | ||
|
||
this.terminal.writeln('Customize your command, then hit ‘return’ to run.'); |
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.
Not super important but return
is one of those OS specific languages at this point. Not sure what the best way tot distinguish enter
vs. return
or if it's even important enough to do.
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 wondered about it and decided to follow what the design had but I don’t love the lack of general term 😐
ui/app/routes/exec.js
Outdated
return this.store | ||
.findRecord('job', fullId, { reload: true }) | ||
.then(job => { | ||
return RSVP.all([job.get('allocations'), job.get('evaluations')]).then(() => job); |
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.
Are evaluations used for exec?
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.
hah they indeed are not, the perils of copypaste! I’ve removed it, thank you.
function encodeString(string) { | ||
let encoded = new TextEncoderLite('utf-8').encode(string); | ||
return base64js.fromByteArray(encoded); | ||
} |
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're gonna need to use this for log and fs streaming as well. We have emoji issues over there.
@@ -0,0 +1,3 @@ | |||
export default function openExecUrl(url) { | |||
window.open(url, '_blank', 'width=973,height=490,location=1'); |
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 wonder if down the road we can do something fancier here by detecting native resolution or something.
ui/public/images/icons/console.svg
Outdated
</g> | ||
</g> | ||
</g> | ||
</svg> |
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.
Ember Inline SVG will do this at build time, but I think this should be run through svgo and saved that way. Just be mindful of preserving the viewBox
.
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.
Makes sense, done, thanks!
|
||
mocks.forEach(route => route(app, options)); | ||
proxies.forEach(route => route(app, options)); | ||
}; |
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.
You'll have to walk me through what's going on here. I'm assuming this is all in an effort to mock web sockets?
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.
yeah, this is boilerplate that I think just replicates what was previous set up in .ember-cli
. I created #7465 so I can revisit this and maybe remove some parts. For instance, I suspect mocks
are superfluous.
test('it generates an exec job URL', function(assert) { | ||
generateExecUrl(this.router, { job: 'job-name' }); | ||
|
||
assert.ok(this.urlForSpy.calledWith('exec', 'job-name', emptyOptions)); |
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.
Cool use of a spy.
The perils of copypaste!
I used this: svgo public/images/icons/console.svg --disable=removeViewBox
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
This is but a prototype, but it works locally! At the moment it won’t work in Mirage and hence in the Netlify deployments, but maybe we can add a minimal toy faux backend for the socket.
Many things remain to be explored, like:
xterm.js
does not support any legacy encoding and probably never will?/bin/bash