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.

Sign up for GitHub

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

Jump to bottom

Initial local UI+dsub docker compose #20

Merged
merged 5 commits into from
Oct 2, 2017
Merged

Initial local UI+dsub docker compose #20

merged 5 commits into from
Oct 2, 2017

Conversation

calbach
Copy link
Contributor

@calbach calbach commented Sep 25, 2017 •
edited
Loading

Resolves #18

  • Starts an nginx proxy, which routes UI requests on / and API requests on /api
  • Works only for local docker provider currently
  • Add some plumbing for UI to API server
  • Reload websocket doesn't currently work in this configuration (see Allowing different HOST headers to be used in ng serve angular/angular-cli#6349)
  • Switch QueryJobs to use a body parameter, which gets passed through to connexion (where parameters doesn't).

@calbach calbach added the WIP label Sep 25, 2017
@calbach
Copy link
Contributor Author

calbach commented Sep 25, 2017

@bfcrampton @alahwa FYI I'm going to need to do a bit more cleanup work and testing on this PR, but exported in case you want to patch and try it out.

@bfcrampton bfcrampton changed the base branch from bc/dsub-shim-implementation to master September 25, 2017 20:05
@bfcrampton bfcrampton force-pushed the ch/compose-merge branch 2 times, most recently from 46138de to 19cae6a Compare September 25, 2017 20:28
@bfcrampton
Copy link

bfcrampton commented Sep 25, 2017 •
edited
Loading

Nice work! Rebased on new changes in master for playing around.

@calbach calbach changed the base branch from master to ch/regen-ts September 28, 2017 19:47
@calbach calbach changed the base branch from ch/regen-ts to ch/dstat-star September 28, 2017 19:59
@calbach calbach removed the WIP label Sep 28, 2017
bfcrampton
bfcrampton requested changes Sep 28, 2017
View reviewed changes
ui/src/app/job-monitor.service.ts Outdated
listAllJobs(): Promise<QueryJobsResponse> {
return this.http.get(`${this.apiUrl}/jobs`,
new RequestOptions({headers: this.headers}))
listAllJobs(parentId?: string): Promise<JobQueryResponse> {
Copy link

Choose a reason for hiding this comment

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

I think you accidentally reverted this, should still be QueryJobsResponse.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right you are - unreverted.

@calbach
Copy link
Contributor Author

calbach commented Sep 28, 2017

Per offline discussion, also changed the API url to be /api/v1.

bfcrampton
bfcrampton approved these changes Sep 28, 2017
View reviewed changes
Copy link

@bfcrampton bfcrampton left a comment

Choose a reason for hiding this comment

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

Two minor nits but otherwise looks good!

ui/src/app/job-monitor.service.ts Outdated
@@ -8,14 +8,20 @@ import {QueryJobsResponse} from './model/QueryJobsResponse';
/** Service wrapper for accessing the job monitor API. */
@Injectable()
export class JobMonitorService {
private apiUrl = '/v1';
private apiUrl = '/api/v1';
Copy link

Choose a reason for hiding this comment

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

Could this be an env variable in docker-compose.yml for the ui service? Or, alternatively, use a shared env_file for both services.

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 wound up using the environment.ts file. I didn't find an obvious standard way to plumb a flag through to the server. Seems like the normal thing is to load up a JSON file or something if you need server configuration. Didn't both consolidating the env var in docker-compose since I wouldn't be able to plumb it here anyways.

servers/dsub/jobs/__main__.py Outdated
parser.add_argument(
'--path_prefix',
type=str,
help='Path prefix, e.g. /api to serve from',
Copy link

Choose a reason for hiding this comment

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

Should we update this help string to /api/v1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, done.

@bfcrampton
Copy link

👍

calbach
calbach commented Sep 28, 2017
View reviewed changes
Copy link
Contributor Author

@calbach calbach left a comment

Choose a reason for hiding this comment

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

Oops, didn't send my buffered comments.

ui/src/app/job-monitor.service.ts Outdated
@@ -8,14 +8,20 @@ import {QueryJobsResponse} from './model/QueryJobsResponse';
/** Service wrapper for accessing the job monitor API. */
@Injectable()
export class JobMonitorService {
private apiUrl = '/v1';
private apiUrl = '/api/v1';
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 wound up using the environment.ts file. I didn't find an obvious standard way to plumb a flag through to the server. Seems like the normal thing is to load up a JSON file or something if you need server configuration. Didn't both consolidating the env var in docker-compose since I wouldn't be able to plumb it here anyways.

@calbach calbach added the WIP label Sep 29, 2017
@calbach calbach removed the WIP label Sep 30, 2017
@calbach calbach changed the base branch from ch/dstat-star to master September 30, 2017 00:21
@calbach
Copy link
Contributor Author

calbach commented Sep 30, 2017

@alahwa PTAL

@calbach calbach mentioned this pull request Sep 30, 2017
Closed
alahwa
alahwa approved these changes Oct 2, 2017
View reviewed changes
Copy link
Contributor

@alahwa alahwa left a comment

Choose a reason for hiding this comment

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

Looks good, nice work!

@calbach calbach merged commit db05d90 into master Oct 2, 2017
@calbach calbach deleted the ch/compose-merge branch October 2, 2017 16:57
alahwa added a commit that referenced this pull request Oct 2, 2017
@alahwa
Merge with changes in #20
46bff83
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bfcrampton bfcrampton bfcrampton approved these changes

@alahwa alahwa alahwa approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@calbach @bfcrampton @alahwa