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.

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

Angular2 webdriver test updates #2038

Merged
merged 8 commits into from
Sep 14, 2016
Merged

Angular2 webdriver test updates #2038

merged 8 commits into from
Sep 14, 2016

Conversation

thompsonja
Copy link
Contributor

@enisoc

This supersedes PR #1988. I'll work on potential Travis caching and/or Dockerizing in a later PR.

@@ -1,12 +1,13 @@
#!/usr/bin/env python
"""A vtctld2 webdriver test."""
"""A vtctld2 webdriver test that tests the different views of status page."""
Copy link
Contributor

Choose a reason for hiding this comment

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

You can keep the old more generic description here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@thompsonja
Copy link
Contributor Author

Also went ahead and checked in the compiled angular2 app into web/vtctld2/app.

@michael-berlin
Copy link
Contributor

Josh, can you please write a minimal script to copy the compiled Angular 2 app and also add a Makefile target for it?

This way it's clear how to update the app.

@enisoc enisoc self-assigned this Sep 13, 2016
@thompsonja
Copy link
Contributor Author

A script isn't really necessary, running make build_web should do the trick as long as bootstrap_web.sh was run.

@enisoc-bot
Copy link

enisoc-bot commented Sep 13, 2016

LGTM but +1 to Michael's suggestion.


Reviewed 4 of 4 files at r1, 3 of 5 files at r2, 1 of 3 files at r3, 15 of 16 files at r4, 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

Approved with PullApprove

@michael-berlin
Copy link
Contributor

Josh, maybe some context here:

The idea is to decouple the checked in version of the app from the own you're currently developing and working one.

The later one should not be under version control and it should not show up as a changed file when you rebuild it.

Therefore, we'll need a script which copies the current state to a dedicated folder where the "release" version of the app lives.

Does that make sense?

@enisoc-bot
Copy link

:lgtm:


Reviewed 3 of 3 files at r6.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@michael-berlin
Copy link
Contributor

Anthony said that we already have this distinction with the dist and the app folder and that only bootstrap_web.sh copies a new version to the app folder?

Just for the record: Which step in bootstrap_web.sh re-generates the app version?

@thompsonja
Copy link
Contributor Author

thompsonja commented Sep 13, 2016

Ok so here is what I'm thinking the flow is for someone making changes to the web app:

  • Run bootstrap.sh
  • Source dev.env
  • Run bootstrap_web.sh
  • Run make build_web whenever you need to rebuild to test a code change. This generates the build in the web/vtctld2/dist/ folder.
  • Test using local example, vtcombo, etc., passing in web/vtctld2/dist as web_dir2.
  • When happy, run tools/generate_web_artifacts.sh, which builds artifacts at web/vtctld2/app/ and removes the unnecessary assets folder.

👍 ? 👎 ?

@mberlin-bot
Copy link

Review status: 21 of 23 files reviewed at latest revision, 2 unresolved discussions.


tools/generate_web_artifacts.sh, line 14 at r7 (raw file):

vtctld2_dir=$VTTOP/web/vtctld2
rm -rf $vtctld2_dir/app

Please make this conditional on the existance of the directory:

if [[ -d $vtctld2_dir/app ]]; then
  rm -rf $vtctld2_dir/app
fi

Comments from Reviewable

@michael-berlin
Copy link
Contributor

michael-berlin commented Sep 13, 2016

I've added a thumbs up to your comment :D

LGTM after comment is addressed :)

Approved with PullApprove

@thompsonja-bot
Copy link

Review status: 21 of 23 files reviewed at latest revision, 2 unresolved discussions.


tools/generate_web_artifacts.sh, line 14 at r7 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

Please make this conditional on the existance of the directory:

if [[ -d $vtctld2_dir/app ]]; then
  rm -rf $vtctld2_dir/app
fi
Done.

Comments from Reviewable

@thompsonja thompsonja merged commit 30b6d1a into vitessio:master Sep 14, 2016
@thompsonja thompsonja deleted the angular branch September 14, 2016 00:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants