Store root directory in variable by DunnCoding · Pull Request #32 · Kitura/KituraKit

DunnCoding · 2018-05-18T09:13:15Z

This PR is to remove changing directories via relative paths.

Instead we store the root directory of the project in a variable, and use the absolute path of the stored pwd

codecov-io · 2018-05-18T09:17:22Z

Codecov Report

Merging #32 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop      #32   +/-   ##
========================================
  Coverage    80.15%   80.15%           
========================================
  Files            2        2           
  Lines          131      131           
========================================
  Hits           105      105           
  Misses          26       26

Flag	Coverage Δ
#KituraKit	`80.15% <ø> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d86140...973ec92. Read the comment docs.

djones6

Please also rebase this branch on develop to get rid of the spurious merge commit from master

djones6 · 2018-05-18T09:22:31Z

ci/update.sh

 git checkout pod
 git pull origin master 

+DIR=$(pwd)


This should be the first line of the script (other than perhaps a comment block that says what this script is for). In case, for example, one of the other commands changed the pwd.

I'd also suggest a comment explaining what the variable is (ie. 'The root of the project directory') and giving it a more descriptive name eg. projectDir

djones6 · 2018-05-18T09:23:58Z

ci/update.sh

@@ -75,7 +77,7 @@ cd ../SwiftyRequest*
 rm -rf ../../../Sources/KituraKit/SwiftyRequest
 cp -r Sources/SwiftyRequest ../../../Sources/KituraKit


These shouldn't be relative paths, they should be using $projectDir (see comment on naming above)

djones6 · 2018-05-18T09:24:37Z

ci/update.sh

+cd $DIR/Sources/KituraKit

 # Remove all the import statements that aren't needed 
 read -a SWIFTFILES <<< $(find .. -name "*.swift")


This should be find $projectDir/Sources, and you can get rid of the cd above altogether

djones6 · 2018-05-18T09:26:17Z

ci/update.sh

+cd $DIR

 rm -rf swift-4.0-RELEASE-ubuntu14.04/
 rm -rf Package-Builder/


similarly you can get rid of the cd and pass an absolute path to rm

DunnCoding · 2018-05-18T09:39:38Z

Addressed all review comments.
@djones6

djones6

Some more instances of cd and ..

djones6 · 2018-05-18T09:46:57Z

ci/update.sh

@@ -59,26 +62,25 @@ git pull origin master
 swift package resolve

 cd .build/checkouts/LoggerAPI*


Unnecessary cd

djones6 · 2018-05-18T09:47:00Z

ci/update.sh

+rm -rf $projectDir/Sources/KituraKit/LoggerAPI
+cp -r Sources/LoggerAPI $projectDir/Sources/KituraKit

 cd ../CircuitBreaker*


Unnecessary cd

djones6 · 2018-05-18T09:47:12Z

ci/update.sh

+rm -rf $projectDir/Sources/KituraKit/CircuitBreaker
+cp -r Sources/CircuitBreaker $projectDir/Sources/KituraKit

 cd ../KituraContracts*


Unnecessary cd

djones6 · 2018-05-18T09:47:20Z

ci/update.sh

+cp -r  Sources/KituraContracts $projectDir/Sources/KituraKit
+mv $projectDir/Sources/KituraKit/KituraContracts/CodableQuery/*.swift $projectDir/Sources/KituraKit/KituraContracts/

 cd ../SwiftyRequest*


Unnecessary cd

djones6 · 2018-05-18T09:49:32Z

ci/update.sh

-rm -rf ../../../Sources/KituraKit/LoggerAPI
-cp -r Sources/LoggerAPI ../../../Sources/KituraKit
+rm -rf $projectDir/Sources/KituraKit/LoggerAPI
+cp -r Sources/LoggerAPI $projectDir/Sources/KituraKit


rather than the cd above, I believe this could be accomplished with

cp -r $projectDir/.build/checkouts/LoggerAPI*/Sources/LoggerAPI $projectDir/Sources/KituraKit

djones6 · 2018-05-18T10:58:06Z

After discussion, the suggestion above doesn't work (apparently, can't have a wildcard in the middle of a file path). An alternative that still avoids .. would be:

 cd $projectDir/.build/checkouts/SwiftyRequest* && cp <source> <dest>
 cd $projectDir/.build/checkouts/LoggerAPI* && cp <source> <dest>

By adding the and (&&) this avoids copying a relative path from the wrong location if the cd were to fail (which is possible, since you are cd'ing into a wildcarded location, where the location is SPM's implementation detail (we've been advised before not to rely on the structure of .build).

A slower (but safer) approach would be:

swift package edit SwiftyRequest
cp $projectDir/Packages/SwiftyRequest/Sources $projectDir/Sources

The location of the edited package is something we can rely on and since it has a predictable name, doesn't require the wildcard stuff above.

djones6 · 2018-05-25T10:22:54Z

ci/update.sh

-rm -rf ../../../Sources/KituraKit/CircuitBreaker
-cp -r Sources/CircuitBreaker ../../../Sources/KituraKit
+rm -rf $projectDir/Sources/KituraKit/CircuitBreaker
+cd .build/checkouts/CircuitBreaker* && cp -r Sources/CircuitBreaker $projectDir/Sources/KituraKit


No good - you are cding to another location above but not returning to $projectDir before executing the next cd to a relative location.
You could fix this by prepending $projectDir to each cd, but then the git commands at the end would fail (you'd need a final cd $projectDir before them).

I'd recommend the cd-less approach I suggested in my earlier review (using swift package edit) as you can then stay at the project root for the duration of the script.

DunnCoding · 2018-06-25T15:03:58Z

@djones6 Ready for a fresh review.
As per your suggestion I've removed the 'cd' commands and opted for the swift package edit option. You mentioned this is a slower option, however as this is a 'travis only' script I think the added safety far outweighs the loss in speed.

Thanks for the suggestion 👍

djones6

👍

* fix: pod update script (#32) * feat: Add basic, google token and facebook token authentication (#33)

DunnCoding requested a review from djones6 May 18, 2018 09:18

djones6 suggested changes May 18, 2018

View reviewed changes

Remove relative paths

3cd7dd8

DunnCoding force-pushed the fixScript branch from f3ee087 to 3cd7dd8 Compare May 18, 2018 09:30

David Dunn added 2 commits May 18, 2018 10:36

Remove all relative paths & add comments

a88311d

Remove uneeded 'cd'

4b5f430

djones6 suggested changes May 18, 2018

View reviewed changes

Fix scripting issues

edf0190

djones6 suggested changes May 25, 2018

View reviewed changes

Remove 'cd' commands

973ec92

djones6 approved these changes Jun 25, 2018

View reviewed changes

DunnCoding merged commit 0f4a8f3 into develop Jun 26, 2018

DunnCoding deleted the fixScript branch June 26, 2018 07:36

djones6 mentioned this pull request Jun 27, 2018

Release 0.0.22 #34

Merged

DunnCoding pushed a commit that referenced this pull request Jun 28, 2018

Release 0.0.22 (#34)

2e5f528

* fix: pod update script (#32) * feat: Add basic, google token and facebook token authentication (#33)

		@@ -75,7 +77,7 @@ cd ../SwiftyRequest*
		rm -rf ../../../Sources/KituraKit/SwiftyRequest
		cp -r Sources/SwiftyRequest ../../../Sources/KituraKit

		@@ -59,26 +62,25 @@ git pull origin master
		swift package resolve

		cd .build/checkouts/LoggerAPI*

Conversation

DunnCoding commented May 18, 2018

Uh oh!

codecov-io commented May 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

djones6 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DunnCoding commented May 18, 2018

Uh oh!

djones6 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

djones6 commented May 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DunnCoding commented Jun 25, 2018

Uh oh!

djones6 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-io commented May 18, 2018 •

edited

Loading