San 3912 dockerfile mirroing #1553
Conversation
…al-setup' into SAN-3912-dockerfile-mirroing * SAN-3925-refactor-container-creation-code-allow-new-modal-setup: Fetch template servers on load. Change addRepoTab to tabName fix scrolling in new container modal
…ockerfile-mirroing * edit-and-setup-modal-use-same-template: Add number of open tabs for animation Fix tests Delete unnecesary template Adding formm bound to controller. Fixing command loaing. Removing unnecesary ng-if Add default state for setupServerModalController Fix header for setupServerModalController Add setupServerModalController tabs/buttons Change editServerModal to use new serverModalView Add tests 4.4.1 Fixed overzealous validation cleanup. Fixed tests. Removed dockerfile validation entirely. This should fix the browser crashing when the maxLineLength is above 2000. 4.4.0 remove pseudo element override intercom previews
thejsj
force-pushed
the
SAN-3912-dockerfile-mirroing
branch
from
53882a9 to
0259200
Compare
thejsj
force-pushed
the
SAN-3912-dockerfile-mirroing
branch
from
0259200 to
4a5f7d4
Compare
thejsj
force-pushed
the
SAN-3912-dockerfile-mirroing
branch
from
ceecd18 to
c1ed103
Compare
| buildDockerfilePath: dockerfile.path | ||
| })) | ||
| .then(function () { | ||
| state.advanced = true; |
There was a problem hiding this comment.
I'm starting to think that having these properties being separate, but having isMirroringDockerfile dependent on advanced, may lead to issues. Can we make state.advanced === 'isMirroringDockerfile' or something, so we have 1 variable, instead of a bunch of flags? Like, I want to prevent ever having to worry about state.advanced = false && state.isMirroringDockerfile = true
thejsj
force-pushed
the
SAN-3912-dockerfile-mirroing
branch
from
57ba88a to
f7da8e9
Compare
thejsj
force-pushed
the
SAN-3912-dockerfile-mirroing
branch
from
521c42c to
904d02c
Compare
| ng-disabled = "" | ||
| placeholder = "Search" | ||
| ng-keyup = "$digest" |
There was a problem hiding this comment.
Um, is this going to trigger a digest on key-up?
There was a problem hiding this comment.
Yes, because the modal needs to get re-rendered when you type something. If not, you get weird heights on the modal.
There was a problem hiding this comment.
fuck..... so we definitely need to figure out a real fix for this. Sounds like it's an issue with our animatedPanel
There was a problem hiding this comment.
I agree this is probably the best way to do it, but I don't think we'll get away with not running a digest cycle every time someone types something, right?
| @@ -3,24 +3,87 @@ | |||
| require('app') | |||
| .controller('ServerModalController', ServerModalController); | |||
|
|
|||
| var TAB_VISIBILITY = { | |||
There was a problem hiding this comment.
Could we have this and the check that's in this controller as it's own service?
| @@ -127,24 +133,47 @@ function NewContainerModalController( | |||
| .catch(errs.handler); | |||
| }; | |||
|
|
|||
| NCMC.setRepo = function (repo) { | |||
| NCMC.setRepo = function (repo, cb, cbParam) { | |||
There was a problem hiding this comment.
cb and cbParam? Ew, gross. Why?
There was a problem hiding this comment.
because you can't use .bind in a template. Will rename to make more clear.
| if (buildDockerfilePath && repoFullName) { | ||
| var branchName = keypather.get(acv, 'attrs.branch'); | ||
| // Get everything before the last '/' and add a '/' at the end | ||
| var path = buildDockerfilePath.replace(/^(.*)\/.*$/, '$1') + '/'; |
There was a problem hiding this comment.
How does changing the capture group effect this replace? Do you want to just do
var result = /^([^\/]+)\/([^\/]+)$/.exec()
var path = result[1]
var name = result[2]
That's a lot easier, imo
There was a problem hiding this comment.
also, you should use [^\/]+, not .*
There was a problem hiding this comment.
Yeah, that makes more sense.
|
Just tried on Epsilon: |
| @@ -41,9 +41,10 @@ function fetchDockerfileForContextVersion ( | |||
| if (buildDockerfilePath && repoFullName) { | |||
| var branchName = keypather.get(acv, 'attrs.branch'); | |||
| // Get everything before the last '/' and add a '/' at the end | |||
| var path = buildDockerfilePath.replace(/^(.*)\/.*$/, '$1') + '/'; | |||
| var result = /^([^\/]*)\/([^\/]*)$/.exec(buildDockerfilePath); | |||
| var path = result && result[1] || ''; | |||
There was a problem hiding this comment.
do a full
if (result.length < 2) {
error
}
…eNow/runnable-angular into SAN-3912-dockerfile-mirroing
|
@thejsj I fixed the disabled items still having an active state. Sorry about that. I messaged Kahn to get more info about the width issue he saw. |
This reverts commit f51cbd6.
…nable-angular into SAN-3912-dockerfile-mirroing * 'SAN-3912-dockerfile-mirroing' of github.com:CodeNow/runnable-angular: fix active state on disabled list items
Changes
SetupServerModalControllerEditServerModalControllerSetupMirrorServerModalControllerFlags
dockerFilMirroringDependencies
Reviewers