Skip to content

Conversation

@naveedahd
Copy link
Contributor

@naveedahd naveedahd commented Jul 31, 2020

The following work was done as a part of this PR:

  1. github-release-resource was implemented to pull the latest release tag from Spring cloud config server's repo. This by default pulls down the assets from the latest release.
  2. This PR has the config server broker's tag manually set to v3.0.0 to test the functionality of the implemented logic. Would be taken out before the merge.
  3. ci/scripts/deploy-testflight script was modified to update the release tag for the config server broker. The logic implemented was to grab the version from tag file that the github-resource-release creates. This version was in turn used to update the config_server_release_tag in the CF broker config. The last part was implemented using [yq](https://github.com/mikefarah/yq/) which is a useful command line YAML processor.
  4. Work in progress: To get rid of the .jar that the github-release-resource fetches by default along with the other assets. There are options to ignore the source tarball and source zip files though. Have to look into this.

@naveedahd naveedahd requested a review from bodymindarts July 31, 2020 08:30
@naveedahd naveedahd self-assigned this Jul 31, 2020
@naveedahd naveedahd added the enhancement New feature or request label Jul 31, 2020
Copy link
Contributor

@bodymindarts bodymindarts left a comment

Choose a reason for hiding this comment

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

PR doesn't address need to bump the config_server_release version (via a git commit). Only changes state locally during the 'deploy-testflight' task.

Needs re-thinking

basic_plan_id: basic
description: Broker to create config-servers
long_description: Broker to create config-servers
config_server_release_tag: v3.1.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Version shouldn't change when merging into master ... perhaps upward if at all

owner: (( param "Please specify the name of the user / organization that owns the Github repository" ))
repo: (( param "Please specify the name of the Github repository" ))
branch: master
branch: update-release-tag
Copy link
Contributor

Choose a reason for hiding this comment

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

branch shouldn't change when merging into master

branch: update-release-tag
private_key: (( param "Please generate an SSH Deployment Key for this repo and specify it here" ))

spring-github:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we find a better name? spring is non descriptive.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps:

cloud-config-server:
  github:
    owner:

private_key: (( param "Please generate an SSH Deployment Key for this repo and specify it here" ))

spring-github:
uri: (( concat "[email protected]:" meta.github.owner "/" meta.github.repo ))
Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't meta.github.repo refer to this repo? (not the java repo)

Copy link
Contributor

Choose a reason for hiding this comment

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

Should be meta.spring-github.owner (or something else once you've changed the name)

uri: (( concat "[email protected]:" meta.github.owner "/" meta.github.repo ))
owner: (( param "Please specify the name of the user / organization that owns the Github repository" ))
repo: (( param "Please specify the name of the Github repository" ))
branch: ci
Copy link
Contributor

Choose a reason for hiding this comment

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

should probably be set in settings.yml NOT here

Copy link
Contributor

Choose a reason for hiding this comment

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

Also oauth is the canonical branch we are currently working off of.

user: (( grab meta.github.owner ))
repository: (( grab meta.github.repo ))
access_token: (( grab meta.github.access_token ))
- name: fetch-spring-latest-rel
Copy link
Contributor

Choose a reason for hiding this comment

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

verb fetch shouldn't be part of resource name. verb is specified in concourse schema eg: {get: <resource-name>}

spring is non-descriptive better to use a name that is more indicative of what we are dealing with.
EG: cloud-config-server-release


cf login -a ${CF_API_URL} --skip-ssl-validation -u ${CF_USERNAME} -p ${CF_PASSWORD} -o ${CF_ORG} -s ${CF_SPACE}

apt-key adv --keyserver keyserver.ubuntu.com --recv-keys CC86BB64
Copy link
Contributor

Choose a reason for hiding this comment

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

installing deps belongs in the definition of the image we are using to execute the jobs. PR: https://github.com/starkandwayne/dockerfiles/tree/master/concourse/latest


latest_release_tag=$(cat fetch-spring-latest-rel/tag)
echo $latest_release_tag
yq w git/cf/broker_config.yml config_server_release_tag $latest_release_tag
Copy link
Contributor

Choose a reason for hiding this comment

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

Normally we do this:

spruce json <file> | jq

@naveedahd naveedahd force-pushed the update-release-tag branch 2 times, most recently from 8f5a61d to 724fd34 Compare August 13, 2020 01:19
@naveedahd
Copy link
Contributor Author

naveedahd commented Aug 18, 2020

@bodymindarts In the past few days, I have tried configuring separate job to bump up config_server_release_version but it seems impossible, at least to me to update that only one variable without invoking all the other spruce operators already present in that file. I have spent a lot of time on this and to me, it seems like we can pull this off in a better fashion using yqin the testflight job itself. My reasoning is that we fetch the cloud-config-server via fetch-spring-latest-releverytime the testflight job is run. Do we really need another separate job to commit the latest version?. Please let me know if you think there is a better way to do it.

@naveedahd
Copy link
Contributor Author

naveedahd commented Aug 18, 2020

There's another method to pull it off, but in the testflight job itself that @dennisjbell helped me with. You can check that quick pull at: Link
For this, we will have to manipulate the file structure/directory so that spruce can actually access the tag file that the fetch-spring-latest-rel resource pulls in, from within the pipeline-tasks dir.

@bodymindarts
Copy link
Contributor

bodymindarts commented Aug 18, 2020

Have you tried using sed?
eg:

$ tag=v3.1.1
$ sed  -i '' "s/config_server_release_tag:.*/config_server_release_tag: ${tag}/" cf/broker_config.yml
$ git diff
diff --git a/cf/broker_config.yml b/cf/broker_config.yml
index 5a40d97..afffd8f 100644
--- a/cf/broker_config.yml
+++ b/cf/broker_config.yml
@@ -7,7 +7,7 @@ basic_plan_name: basic
 basic_plan_id: basic
 description: Broker to create config-servers
 long_description: Broker to create config-servers
-config_server_release_tag: v3.1.0
+config_server_release_tag: v3.1.1
 cloud_foundry_config:
   api_url: (( param "Please override cloud_foundry_confi

Dennis and I spoke about it a while ago (he mentioned he had helped you) and we agreed there must be a commit either before or after TestFlight. Wether or not you do it before or after is a fair disagreement. Either way has its merit - I still prefer before but wouldn't argue with someone that thinks after is better.

Either way the state that you test must be persisted in the repo at some point.

FYI if you do the sed way (which seems the easiest to me) be aware that the -i syntax is slightly different with GNU sed vs MacOs so take that into account (the script is running inside a container - so what works on your Mac might not work there).

UPDATE: using the (( file ... )) spruce operator as per Dennis suggestion is also fine, but there must be a way to commit back the state you have changed or cutting a release is a liability.

@norman-abramovitz
Copy link

Naveed, Are you ready for a review? If so, please request a review again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants