-
Notifications
You must be signed in to change notification settings - Fork 53
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
Staging panel mods [TASK-1134] #1201
Conversation
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 think it's mostly fine, but let's standardize on a single staging service client.
this._super(options); | ||
|
||
this.stagingServiceClient = new StagingServiceClient({ | ||
root : 'https://ci.kbase.us/services/staging_service', |
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 should be taken from Config.url('ftp_api_url')
@@ -52,6 +65,10 @@ define([ | |||
this.fileStagingClient = new FileStaging( | |||
this.ftpUrl, this.userId, {token: runtime.authToken()}); | |||
|
|||
this.genericClient = new GenericClient(Config.url('service_wizard'), { | |||
token: Runtime.make().authToken() |
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.
There's already an initialized local handle on Runtime. So just runtime.authToken()
works here.
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.
All changes made.
- I got rid of references to the FileStaging() client in favor of StagingServiceClient
- It can't use the ftp url, since it doesn't have the same routes. I added a staging_api_url to src/config.json, including the fake pointers for appdev, next, and prod services which don't exist yet.
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.
the FTP url is, i guess, deprecated at this point. we can remove it later. I'm planning on scouring through the repo and ditching all kinds of crap - that can be part of it. :)
@@ -52,6 +65,10 @@ define([ | |||
this.fileStagingClient = new FileStaging( |
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 should pick one Staging Service client and roll with it. We both rolled our own. I gotta say, I like yours better because it comes with a more generic REST client that we could use elsewhere.
Hey, that's the same Travis-CI problem that's fixed in #1200! The rest looks fine. We can merge and redeploy if you're ready. |
I'm good to go whenever you are. As long as that travis thing is a non-issue, go ahead and pull the trigger. |
Just as an FYI, at the moment the staging_service is tossing a 500 error on a list/ hit, which means that no files are listed and showing up on the panel. I just tweaked it so that if it fails in that case it reports to the user instead of just logging to the console. I'm pretty sure it's a service issue instead of the widget, since I see the same 500 error if I try to hit the URL directly. It'd probably be good to hold off on merging and deploying this until everything is working properly. |
I wasn't sure of the task to tack onto this, but 1134 seemed reasonable.
This is the initial crack at updating the staging panel. We should get it up onto CI as soon as we can.
It includes:
It lacks:
Right now, I'd just like some more eyes on it to see how it's coming along.