Skip to content

Adding Deploy Image and Import YAML / JSON functionality to catalog #1893

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
Merged
merged 1 commit into from
Aug 16, 2017
Merged

Adding Deploy Image and Import YAML / JSON functionality to catalog #1893

merged 1 commit into from
Aug 16, 2017

Conversation

rhamilto
Copy link
Member

@rhamilto rhamilto commented Jul 28, 2017 •
edited
Loading

https://trello.com/c/Dk9IxmCH

localhost-9000-dev-console-
localhost-9000-dev-console- 1
localhost-9000-dev-console- 2
localhost-9000-dev-console- 3
localhost-9000-dev-console- 4

TODO:

  • utilize ProjectsService in createProjectIfNecessary
  • fix race condition where a new project could be selected before configmaps and secrets return
  • fix incorrect warning when deploying image from image stream into same namespace
  • fix image search for deploying images into new projects
  • improve ux around disabled image search when creating a new project; entering a search query without submitting with a project selected and changing to create a new project is especially bad
  • display an error if trying to create a project using the same name as a recently deleted but not fully deleted project
  • resolve issue where fromValue env vars created under a project display as empty read only inputs when changing to Create Project
    screen shot 2017-08-14 at 3 56 56 pm
  • resolve issue created by fix for fromValue env vars where clicking "Create" fails to advance the wizard to the next step
  • resolve issue where fromValue env vars created under a project display as read only inputs when changing image stream or mode
    screen shot 2017-08-15 at 11 13 26 am

spadgett
spadgett reviewed Aug 1, 2017
View reviewed changes
Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

@rhamilto LGTM. The only problem I see is the race condition loading secrets and config maps, which you've noted.

@spadgett
Copy link
Member

spadgett commented Aug 4, 2017 •
edited
Loading

Seeing this warning when I shouldn't be since the project is the same.

openshift web console 2017-08-04 08-10-52

Edit: I also noticed the message is wrong. It's missing the project in the service account name.

@spadgett
Copy link
Member

spadgett commented Aug 4, 2017

Ran into a problem trying to search for images when creating a new project.

{
  "kind": "Status",
  "apiVersion": "v1",
  "metadata": {},
  "status": "Failure",
  "message": "User \"sam\" cannot create imagestreamimports in project \"test\"",
  "reason": "Forbidden",
  "details": {
    "kind": "imagestreamimports"
  },
  "code": 403
}

@spadgett spadgett mentioned this pull request Aug 4, 2017
Merged
@openshift-bot openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 10, 2017
rhamilto
rhamilto commented Aug 15, 2017
View reviewed changes
app/scripts/directives/deployImage.js Outdated
@@ -242,18 +213,69 @@ angular.module("openshiftConsole")
});
}, true);

$scope.$watch('input.selectedProject', function(project){
// clear any existing valueFrom env to avoid invalid data
// this breaks the wizard when clicking Deploy
Copy link
Member Author

Choose a reason for hiding this comment

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

@spadgett, this is the culprit of the remaining TODO

@openshift-bot openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 16, 2017
@rhamilto rhamilto changed the title [WIP] Adding Deploy Image and Import YAML / JSON functionality to catalog Adding Deploy Image and Import YAML / JSON functionality to catalog Aug 16, 2017
spadgett
spadgett reviewed Aug 16, 2017
View reviewed changes
Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

LGTM, just one comment on the message

app/scripts/directives/fromFile.js Outdated
NotificationsService.addNotification({
id: "import-create-project-error",
type: "error",
message: "An error occured creating project",
Copy link
Member

Choose a reason for hiding this comment

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

"occurred" is misspelled

"An error occurred creating the project."

spadgett
spadgett reviewed Aug 16, 2017
View reviewed changes
app/scripts/directives/fromFile.js Outdated
createProjectIfNecessary().then(function(project) {
$scope.input.selectedProject = project;
$q.all(resourceCheckPromises).then(function() {
if ($scope.errorOccured) {
Copy link
Member

Choose a reason for hiding this comment

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

Actually looks like it's misspelled here in the variable name too but that's not new

spadgett
spadgett reviewed Aug 16, 2017
View reviewed changes
app/scripts/directives/deployImage.js Outdated
NotificationsService.addNotification({
id: "deploy-image-create-project-error",
type: "error",
message: "An error occured creating project",
Copy link
Member

Choose a reason for hiding this comment

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

also here

@spadgett
Copy link
Member

[merge]

@openshift-bot
Copy link

Evaluated for origin web console merge up to 622b129

@openshift-bot
Copy link

openshift-bot commented Aug 16, 2017 •
edited
Loading

Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin_web_console/79/) (Base Commit: 82888c4) (PR Branch Commit: 622b129)

@openshift-bot openshift-bot merged commit 5671a61 into openshift:master Aug 16, 2017
@rhamilto rhamilto deleted the deploy-image-import-yaml branch August 17, 2017 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spadgett spadgett spadgett approved these changes

@jeff-phillips-18 jeff-phillips-18 Awaiting requested review from jeff-phillips-18

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
@rhamilto @spadgett @openshift-bot