Upload generated provisioning profiles to Bitrise #70

BirmacherAkos · 2018-03-21T10:32:55Z

Only the prov. profiles.
The certificates will come on the next round.

- uploaders/common.go: error handling for io.Copy() - uploaders/requestResponse: Doc fix

godrei · 2018-03-23T13:43:04Z

uploaders/common.go

@@ -0,0 +1,322 @@
+package uploaders


the main file's name of a package .go should be the package name: uploaders.go

godrei · 2018-03-23T13:46:01Z

uploaders/common.go

+
+// AskAccessToken ...
+func AskAccessToken() (token string, err error) {
+	messageToAsk := "Please copy your personal access token to Bitrise.\n" +


use '`' syntax to define multi line string:

godrei · 2018-03-23T13:48:02Z

uploaders/common.go

+func createRequestURLForMyApps() string {
+	requestURL, err := urlutil.Join(baseURL, myAppsEndPoint)
+	if err != nil {
+		failWithMessage("failed to create request URL, error: %s", err)


do not call os exit int this kind of small units (its i ok in main())

do not separate this to a new function: it is only used by one other function and has only one fn call

godrei · 2018-03-23T13:54:11Z

uploaders/common.go

+	return req
+}
+
+func createRequest(url string, fields map[string]interface{}, requestMethod httpMethod, headers map[string]string) (*http.Request, error) {


recommended input order: method, url, headers, body

no need to define httpMethod struct (we use only two method: "GET" and "POST" + they are short and well known)

godrei · 2018-03-23T13:55:30Z

uploaders/common.go

+}
+
+func createRequest(url string, fields map[string]interface{}, requestMethod httpMethod, headers map[string]string) (*http.Request, error) {
+	b := new(bytes.Buffer)


no need to explicit initialize b: var b bytes.Buffer works great and shows, that we do not care about its initial value.

godrei · 2018-03-23T13:56:17Z

uploaders/common.go

+	if err != nil {
+		return nil, err
+	}
+	if _, err := io.Copy(os.Stdout, req.Body); err != nil {


why do we copy the body to stdout?

godrei · 2018-03-23T14:00:26Z

uploaders/common.go

+	return requestURL
+}
+
+func createRequestURLForProvProfSlug(appSlug string) string {


do not separate this logic into a new function

godrei · 2018-03-23T14:00:52Z

uploaders/common.go

+	return requestURL
+}
+
+func failWithMessage(format string, v ...interface{}) {


avoid os exiting in package!

godrei · 2018-03-23T14:01:43Z

uploaders/httpMethod.go

@@ -0,0 +1,25 @@
+package uploaders


use snake_case go file names: http_method.go

based on my previous comment: remove this file!

godrei · 2018-03-23T14:08:15Z

uploaders/requestResponse.go

+}
+
+// Appliocation ...
+type Appliocation struct {


type Appliocation

godrei · 2018-03-23T14:09:54Z

uploaders/requestResponse.go

@@ -0,0 +1,45 @@
+package uploaders


may call this file: models.go

godrei · 2018-03-23T14:10:59Z

uploaders/common.go

+	return nil
+}
+
+func createUploadRequest(url string, files map[string]string, requestMethod httpMethod, headers map[string]string) *http.Request {


if there are several 'commonly' used functions you can move them into: common.go file

Are you sure about this?
I think it would be better if every "network type" function will be under the uploaders package and not the cmd package.

godrei · 2018-03-23T14:11:36Z

cmd/common.go

+// ----------------------------------------------------------------
+// --- Upload methods
+
+// TODO


avoid leaving TODO comment in your code!

godrei · 2018-03-23T14:12:48Z

cmd/common.go

+
+// TODO
+func uploadProvisioningProfiles(profilesToUpload []profileutil.ProvisioningProfileInfoModel, outputDirPath string) error {
+	// Get accesToken from stdin


no need for this comment AskAccessToken is a descriptive name

godrei · 2018-03-23T14:14:15Z

cmd/common.go

+// TODO
+func uploadProvisioningProfiles(profilesToUpload []profileutil.ProvisioningProfileInfoModel, outputDirPath string) error {
+	// Get accesToken from stdin
+	accessToken, err := uploaders.AskAccessToken()


AskAccessToken does not belong to uploaders, do it here in common.go

godrei · 2018-03-23T14:15:54Z

cmd/common.go

+	// Get accesToken from stdin
+	accessToken, err := uploaders.AskAccessToken()
+
+	appList, err := uploaders.FetchMyApps(accessToken)


uploaders.FetchMyApps() is a strange function name.

may the uploaders package should be: bitriseclient, this way:

client := bitriseclient.New(AccessToken) client.FetchMyApps() client.UploadProfiles() entc...

creating a Client model may worth, since you need to use AccessToken at every request.

godrei · 2018-03-23T14:16:15Z

cmd/common.go

+
+	// Upload provisioning profiles
+	for _, profile := range profilesToUpload {
+


avoid unnecessary empty lines!

godrei · 2018-03-23T14:18:38Z

cmd/common.go

+
+		exportFileName := provProfileExportFileName(profile, outputDirPath)
+
+		provProfile, err := os.Open(outputDirPath + "/" + exportFileName)


do this size calculation in RegisterProvisioningProfile

godrei · 2018-03-23T14:20:38Z

cmd/common.go

+		}
+
+		err = uploaders.UploadProvisioningProfile(provProfSlugResponseData, outputDirPath, exportFileName)
+		if err != nil {


if function returns only a single error, define and check the error in if statement:

if err := uploaders.UploadProvisioningProfile(provProfSlugResponseData, outputDirPath, exportFileName); err != nil {
}

avoid using err = xyz() this can easily lead to a shadowed error! always try to define the error like: err := xyz()

godrei · 2018-03-23T14:22:39Z

cmd/common.go

+		bytes := info.Size()
+		log.Debugf("\n%s size: %d", exportFileName, bytes)
+
+		provProfSlugResponseData, err := uploaders.RegisterProvisioningProfile(accessToken, selectedApp.Slug, bytes, profile)


it seems we only use RegisterProvisioningProfileResponseData's UploadURL and UploadFileName, return this values in RegisterProvisioningProfile instead of a new model with unused fields

godrei · 2018-03-23T14:24:57Z

cmd/common.go

+	return nil
+}
+
+func selectApp(appList []uploaders.Appliocation) (seledtedApp uploaders.Appliocation, err error) {


please check which fields of the app is used if there is no more then 2 fields may we do not need a struct.

godrei · 2018-03-23T14:25:23Z

cmd/common.go

+}
+
+func selectApp(appList []uploaders.Appliocation) (seledtedApp uploaders.Appliocation, err error) {
+	selectionList := []string{}


if we do not care about initial value, use: var selectionList []string

godrei · 2018-03-23T14:28:42Z

cmd/common.go

+
+	log.Debugf("selected app: %v", userSelection)
+
+	selectedApp, err := getAppFromUserSelection(userSelection, appList)


no need for this function:

sort the selectionList

then find the selected index by: sort.Search...

then appList[i] is the app

also you can return this function: return getAppFromUserSelection

I have changed to the sort.Search() method as you advised, but I have not removed the getAppFromUserSelection() because it has tests.

godrei

please have a look on my comments

Squashed commits: [6a9bc13] PR fix (+3 squashed commits) Squashed commits: [a22d42c] uploaders/requestResponse.go renamed to uploaders/model.go [be88ad1] Application type fix; uploaders.go: failWithMessage removed, Instead every func return with an error if needed; [1da5d56] httpMethod removed;

godrei · 2018-03-29T14:45:21Z

bitriseclient/bitriseclient.go

+
+	// Response struct
+	var appListResponse FetchMyAppsResponse
+	responseStatusCode := -1


do we use it?

godrei · 2018-03-29T14:45:36Z

bitriseclient/certificate.go

+
+	// Response struct
+	requestResponse := FetchUploadedIdentityListResponse{}
+	responseStatusCode := -1


do we use it?

godrei · 2018-03-29T14:45:40Z

bitriseclient/certificate.go

+	log.Debugf("\nRequest URL: %s", requestURL)
+
+	// Response struct
+	requestResponse := FetchUploadedIdentityListResponse{}


godrei · 2018-03-29T14:46:03Z

bitriseclient/certificate.go

+	}
+
+	// Response struct
+	responseStatusCode := -1


do we use it?

godrei · 2018-03-29T14:48:35Z

bitriseclient/profile.go

+		return "", err
+	}
+
+	data, err := profileutil.NewProvisioningProfileInfo(*plistData, profileutil.ProfileTypeIos)


return profileutil.NewProvisioningProfileInfo(*plistData, profileutil.ProfileTypeIos)

We only need the UUID

godrei · 2018-03-29T14:51:56Z

bitriseclient/certificate.go

+	// Perform request
+	if err := retry.Times(1).Wait(5 * time.Second).Try(func(attempt uint) error {
+		body, statusCode, err := performRequest(request)
+		if err != nil {


the thing is: if the request returns non success status code, the erro will be nil.
later you do not check the status code, probably the json marshal would fail, since in case of error the response is something else then the success model, but fail fast!

please check the status code and retry if non success received

You are right, but the performRequest do this already. If the status code != statusOK it will return an error.

godrei · 2018-03-29T14:53:20Z

cmd/common.go

+	return nil
+}
+
+func uploadExportedFiles(profilesToExport []profileutil.ProvisioningProfileInfoModel, certificatesToExport []certificateutil.CertificateInfoModel,


do not use named returns values in this long func

We need it here because this func returns 2 bool attr. and it can cause a misconception.
And if the provisioning profiles are uploaded successfully but the certs. are not, only the first bool (provProfilesUploaded) will be true.

godrei · 2018-03-29T14:54:09Z

cmd/common.go

+	fmt.Println()
+	log.Infof("Looking for certificate duplicates on Bitrise...")
+
+	uploadedCertificatesSerialList := []string{}


godrei · 2018-03-29T14:54:23Z

cmd/common.go

+
+	// Get uploaded certificates' serials
+	for _, uploadedIdentity := range uploadedItentityList {
+		serialListAsString := []string{}


godrei · 2018-03-29T14:59:59Z

cmd/common.go

+			return err
+		}
+
+		bytes := info.Size()


no need for this new var

godrei · 2018-03-29T15:01:09Z

cmd/common.go

+		return err
+	}
+
+	if err := bitriseClient.ConfirmIdentityUpload(certificateResponseData.Slug, certificateResponseData.UploadFileName); err != nil {


return bitriseClient.ConfirmIdentityUpload(certificateResponseData.Slug, certificateResponseData.UploadFileName)

You can do that but I think it will break the code's continuity. (The previous func call looks the same).
That's why I would not change the return statement.

godrei · 2018-03-29T15:01:52Z

cmd/common.go

+	}
+
+	userSelection, err := goinp.SelectFromStringsWithDefault("Select the app which you want to upload the privisioning profiles", 1, selectionList)
+


remove this empty line

godrei · 2018-03-29T15:03:32Z

cmd/common.go

+		return err
+	}
+
+	bytes := info.Size()


no need for new var

godrei · 2018-03-29T15:05:46Z

cmd/common.go

+func askUploadGeneratedFiles() (bool, error) {
+	messageToAsk := "Do you want to upload the provisioning profiles and certificates to Bitrise?"
+
+	answer, err := goinp.AskForBoolFromReader(messageToAsk, os.Stdin)


return goinp.AskForBoolFromReader(messageToAsk, os.Stdin)

godrei · 2018-03-29T15:06:00Z

cmd/common.go

+func askUploadIdentities() (bool, error) {
+	messageToAsk := "Do you want to upload the certificates to Bitrise?"
+
+	answer, err := goinp.AskForBoolFromReader(messageToAsk, os.Stdin)


return goinp.AskForBoolFromReader(messageToAsk, os.Stdin)

godrei · 2018-03-29T15:09:02Z

cmd/common.go

+	fmt.Println()
+	log.Infof("Looking for provisioning profile duplicates on Bitrise...")
+
+	uploadedProfileUUIDList := []string{}


map would be much better for this purpose, no need for IsStringInSlice:
uploadedProfileUUIDList := map[string]bool{}

trapacska · 2018-04-04T08:40:52Z

bitriseclient/bitriseclient.go

+	client := &BitriseClient{accessToken, "", map[string]string{"Authorization": "token " + accessToken}}
+	var apps []Application
+
+	log.Infof("Asking your application list from Bitrise...")


Asking -> fetching

trapacska · 2018-04-04T08:42:07Z

bitriseclient/bitriseclient.go

+
+	for key, value := range headers {
+		req.Header.Add(key, value)
+


please remove unused newlines

trapacska · 2018-04-04T09:12:36Z

bitriseclient/certificate.go

+
+	//
+	// Perform request
+	if err := retry.Times(1).Wait(5 * time.Second).Try(func(attempt uint) error {


Please shorten these long logics.

trapacska · 2018-04-04T09:13:21Z

bitriseclient/certificate.go

+}
+
+// FetchIdentityResponseData ...
+type FetchIdentityResponseData struct {


please shorten model names and also remove un-relevant part of the names

trapacska · 2018-04-04T09:14:02Z

bitriseclient/bitriseclient.go

+}
+
+// NewBitriseClient ...
+func NewBitriseClient(accessToken string) (*BitriseClient, []Application, error) {


Please try to create an authenticated httpClient so you won't need to create it all the time and set the auth header for that.

Squashed commits: [e108a55] BitriseClient: RunRequest method (+2 squashed commits) Squashed commits: [53c1dba] Renames [181bf12] FetchIdentityListResponse rename

trapacska · 2018-04-04T13:04:10Z

cmd/common.go

+}
+
+func uploadExportedFiles(profilesToExport []profileutil.ProvisioningProfileInfoModel, certificatesToExport []certificateutil.CertificateInfoModel,
+	outputDirPath string) (provProfilesUploaded bool, certsUploaded bool, err error) {


Please separate the functionality into two functions rather than merging the return values together.

… and to uploadIdentity;

BirmacherAkos force-pushed the developer branch from a72d65a to 1770629 Compare March 23, 2018 13:12

codesign files upload

a76b279

BirmacherAkos force-pushed the developer branch from 1770629 to a76b279 Compare March 23, 2018 13:13

- cmd/common.go: Merge fix

770cc22

- uploaders/common.go: error handling for io.Copy() - uploaders/requestResponse: Doc fix

godrei reviewed Mar 23, 2018

View reviewed changes

uploaders/requestResponse.go Outdated

}

// Appliocation ...

type Appliocation struct {

Copy link

Contributor

godrei Mar 23, 2018 •

edited

Loading

Choose a reason for hiding this comment

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

type Appliocation

godrei reviewed Mar 23, 2018

View reviewed changes

uploaders/requestResponse.go Outdated

@@ -0,0 +1,45 @@

package uploaders

Copy link

Contributor

godrei Mar 23, 2018

Choose a reason for hiding this comment

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

may call this file: models.go

godrei reviewed Mar 23, 2018

View reviewed changes

cmd/common.go

// Upload provisioning profiles

for _, profile := range profilesToUpload {

Copy link

Contributor

godrei Mar 23, 2018

Choose a reason for hiding this comment

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

avoid unnecessary empty lines!

godrei reviewed Mar 23, 2018

View reviewed changes

godrei requested changes Mar 23, 2018

View reviewed changes

BirmacherAkos added 2 commits March 23, 2018 15:35

uploaders/common.go renamed to uploaders/uploaders.go

6d7c69e

godrei reviewed Mar 29, 2018

View reviewed changes

bitriseclient/certificate.go Outdated

}

// Response struct

responseStatusCode := -1

Copy link

Contributor

godrei Mar 29, 2018

Choose a reason for hiding this comment

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

do we use it?

godrei reviewed Mar 29, 2018

View reviewed changes

cmd/common.go Outdated

return err

}

bytes := info.Size()

Copy link

Contributor

godrei Mar 29, 2018

Choose a reason for hiding this comment

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

no need for this new var

godrei reviewed Mar 29, 2018

View reviewed changes

cmd/common.go Outdated

return err

}

bytes := info.Size()

Copy link

Contributor

godrei Mar 29, 2018

Choose a reason for hiding this comment

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

no need for new var

godrei reviewed Mar 29, 2018

View reviewed changes

PR fix - clean

a24169f

trapacska reviewed Apr 4, 2018

View reviewed changes

bitriseclient/bitriseclient.go Outdated

for key, value := range headers {

req.Header.Add(key, value)

Copy link

trapacska Apr 4, 2018

Choose a reason for hiding this comment

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

please remove unused newlines

trapacska reviewed Apr 4, 2018

View reviewed changes

BitriseClient: get a http.Client{} (+1 squashed commit)

773b09c

Squashed commits: [e108a55] BitriseClient: RunRequest method (+2 squashed commits) Squashed commits: [53c1dba] Renames [181bf12] FetchIdentityListResponse rename

BirmacherAkos force-pushed the developer branch from c5d9fe4 to 773b09c Compare April 4, 2018 11:14

trapacska reviewed Apr 4, 2018

View reviewed changes

BirmacherAkos added 4 commits April 5, 2018 11:14

- cmd/common.go: uploadExportedFiles() splitted to uploarProvProfiles…

790ae43

… and to uploadIdentity;

-

79963ce

Clean: newlines removed

a5a5c17

clean

bb21679

trapacska merged commit 7045fd3 into bitrise-io:master Apr 6, 2018


		// Upload provisioning profiles
		for _, profile := range profilesToUpload {


		exportFileName := provProfileExportFileName(profile, outputDirPath)

		provProfile, err := os.Open(outputDirPath + "/" + exportFileName)


		log.Debugf("selected app: %v", userSelection)

		selectedApp, err := getAppFromUserSelection(userSelection, appList)

		}

		userSelection, err := goinp.SelectFromStringsWithDefault("Select the app which you want to upload the privisioning profiles", 1, selectionList)

Upload generated provisioning profiles to Bitrise #70

Upload generated provisioning profiles to Bitrise #70

Uh oh!

Conversation

BirmacherAkos commented Mar 21, 2018

Uh oh!

godrei Mar 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

godrei Mar 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

godrei Mar 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

godrei Mar 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

godrei Mar 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

godrei Mar 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

godrei Mar 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

godrei Mar 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

godrei 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

godrei Mar 23, 2018 •

edited

Loading

godrei Mar 23, 2018 •

edited

Loading

godrei Mar 23, 2018 •

edited

Loading

godrei Mar 23, 2018 •

edited

Loading

godrei Mar 23, 2018 •

edited

Loading

godrei Mar 23, 2018 •

edited

Loading

godrei Mar 23, 2018 •

edited

Loading

godrei Mar 23, 2018 •

edited

Loading