Add GA code #145

shammamah-zz · 2019-06-11T15:45:57Z

About

Moved git user/email config out of the if conditional for R apps
Used CircleCI environment variable to directly write to GA file in assets/ folder
Changed CircleCI job to fail if there is no assets/ folder in the app directory
Removed unnecessary code from the predeploy script (R apps now have the files copied over from within the Circle job and consequently don't need to be part of the predeploy script)
Updated README to reflect that only Python apps can use the app name mapping file

rpkyle

💃

rpkyle · 2019-06-11T19:06:06Z

predeploy.py

-file_list = rfiles if "-" in app_name and app_name.split("-")[0] == "dashr" else pyfiles
-
-for f in file_list:
+for f in pyfiles:


👍 I think it's safe to 🔪 the R-related lines here, given that @tarzzz's fix uses the pre-predeploy.py CircleCI test flow to handle R app deployments.

rpkyle · 2019-06-11T19:10:04Z

README.md

@@ -46,7 +46,7 @@ There are two options when you are naming the folder:

 1. Make the folder have the _exact same_ name as the Dash app name.

-2. Select any other name, but _update the file
+2. (Python apps only) Select any other name, but _update the file
   [`apps_mapping.py`](apps_directory_mapping.py)_ with the Dash app
   name and the folder name you have selected.



I will open a second PR to update the README.md with the additional details that are currently required for R app deployments.

nicolaskruchten

I'm not familiar enough with this code to give a thorough review but it seems OK to me.

tarzzz · 2019-06-17T23:18:19Z

.circleci/config.yml

@@ -35,6 +35,7 @@ jobs:
          name: Deploy
          command: |
            APPS_MODIFIED=$(git diff origin/master origin/$CIRCLE_BRANCH --dirstat=files,1 apps/ | awk '{ split($2,a,"/"); if (length(a[2]) != 0) { print a[2]} } ' | sort -u)
+	    export PLOTLY_GA_CODE=$PLOTLY_GA_CODE


Please clean up the here.

tarzzz · 2019-06-17T23:20:19Z

.circleci/config.yml

@@ -35,6 +35,8 @@ jobs:
          name: Deploy
          command: |
            echo $PLOTLY_GA_CODE > ga_code.txt
+	    cat ga_code.txt


If it is confidential, it is not a good idea to cat it here. if it is not, I dont see a reason why it needs to go to circle CI environment variables.

tarzzz · 2019-06-17T23:20:59Z

predeploy.py

@@ -20,7 +20,7 @@

 for f in file_list:
    shutil.copyfile(os.path.join(app_path, f), f)
-    
+


Would be good to squash these commits into one because it is easier for the reviewer.

Change tabs

Change tabs to spaces

Change all tabs to spaces and copy GA code directly into assets folder.

Untabify

tarzzz · 2019-06-18T20:37:08Z

.circleci/config.yml

-            sudo pip install black
+	  name: install black
+	  command: |
+	    sudo pip install black


Here, you are actually converting spaces to tabs. Might want to fix that..

tarzzz · 2019-06-18T20:38:57Z

.circleci/config.yml

+	    then
+		  echo "No app change detected. Skipping the deploy.."
+		  exit 0
+	    fi



This whole commit seems to be doing opposite of what you mentioned in the commit message (ie, "change tabs to spaces"). You might want to remove this commit or clean it up.

tarzzz

Looks good.. 💃
Some notes for the future PRs:

Please keep your commits atomic. You seem to be doing two very unrelated things in one commit.
Please keep your commit history clean. If you are doing something in one commit and then reverting it in another, you should squash the two commits into one.

shammamah-zz · 2019-06-19T02:53:48Z

@tarzzz Thank you! I'll make sure to do that in the future.

shammamah-zz requested review from rpkyle and nicolaskruchten June 11, 2019 18:49

rpkyle previously approved these changes Jun 11, 2019

View reviewed changes

shammamah-zz changed the title ~~Add test app~~ Add GA code Jun 12, 2019

nicolaskruchten reviewed Jun 12, 2019

View reviewed changes

shammamah-zz requested a review from tarzzz June 13, 2019 15:15

tarzzz reviewed Jun 17, 2019

View reviewed changes

Shammamah Hossain added 8 commits June 18, 2019 08:54

Use file instead of environment variable.

f1bd089

Move order of predeploy commands and add variable echo

247db77

Change tabs

Add ga code to git

a7eed4e

Change tabs to spaces

Add user/email for git

f7c1fe4

Change all tabs to spaces and copy GA code directly into assets folder.

Remove R file copying from predeploy script.

5707884

Untabify

Fix typo in if statement.

e03cae0

Add google analytics to gallery deployments.

9a73c98

Clarify that only Python apps can use the app directory mapping.

4a5d3b0

shammamah-zz dismissed rpkyle’s stale review via 4a5d3b0 June 18, 2019 12:57

shammamah-zz force-pushed the dash-app-test branch from 726a2d1 to 4a5d3b0 Compare June 18, 2019 12:57

Merge branch 'master' into dash-app-test

359f437

shammamah-zz requested a review from tarzzz June 18, 2019 12:59

Merge branch 'master' into dash-app-test

6641970

tarzzz reviewed Jun 18, 2019

View reviewed changes

tarzzz approved these changes Jun 18, 2019

View reviewed changes

shammamah-zz merged commit 1bed23c into master Jun 19, 2019

This was referenced Jun 19, 2019

Dummy file for redeploy #153

Merged

Force redeploy #157

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GA code #145

Add GA code #145

shammamah-zz commented Jun 11, 2019 •

edited

rpkyle left a comment

rpkyle Jun 11, 2019

rpkyle Jun 11, 2019

nicolaskruchten left a comment

tarzzz Jun 17, 2019

tarzzz Jun 17, 2019

tarzzz Jun 17, 2019

tarzzz Jun 18, 2019

tarzzz Jun 18, 2019

tarzzz left a comment •

edited

shammamah-zz commented Jun 19, 2019

		@@ -20,7 +20,7 @@

		for f in file_list:
		shutil.copyfile(os.path.join(app_path, f), f)

Add GA code #145

Add GA code #145

Conversation

shammamah-zz commented Jun 11, 2019 • edited

About

rpkyle left a comment

Choose a reason for hiding this comment

rpkyle Jun 11, 2019

Choose a reason for hiding this comment

rpkyle Jun 11, 2019

Choose a reason for hiding this comment

nicolaskruchten left a comment

Choose a reason for hiding this comment

tarzzz Jun 17, 2019

Choose a reason for hiding this comment

tarzzz Jun 17, 2019

Choose a reason for hiding this comment

tarzzz Jun 17, 2019

Choose a reason for hiding this comment

tarzzz Jun 18, 2019

Choose a reason for hiding this comment

tarzzz Jun 18, 2019

Choose a reason for hiding this comment

tarzzz left a comment • edited

Choose a reason for hiding this comment

shammamah-zz commented Jun 19, 2019

shammamah-zz commented Jun 11, 2019 •

edited

tarzzz left a comment •

edited