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

added a python script to validate the sonar token before it is generated #65

Merged
merged 1 commit into from
Aug 21, 2018
Merged

added a python script to validate the sonar token before it is generated #65

merged 1 commit into from
Aug 21, 2018

Conversation

RobeDevOps
Copy link
Contributor

  • Added new python script to validate the sonar token generation
  • If token exists then the process of generation is skipped.

This issue is fixed based on
Accenture/adop-docker-compose#285

@RobeDevOps
Copy link
Contributor Author

Please let me know who can be the reviewer and if anything needs to be fixed or improved.

@dsingh07
Copy link
Contributor

Hi @RobeDevOps Thank you for the PR! Looks good to me!

Just wondering if it's possible to extend the existing bash script to deal with the case where a token already exists rather than needing an extra python script to do the checking? I think it will make it easier to debug and decipher if all the logic for the sonar auth token generation is encapsulated within one script. What do you think?

/CC @RobertNorthard

@RobeDevOps
Copy link
Contributor Author

RobeDevOps commented Aug 15, 2018

I am ok with you if you are ok installing jq in order to iterate the json array using bash script? Using jq should be easy to iterate the token list extending the same bash. Let me know and I will add jq to the tools and implement the solution into the script.

@dsingh07
Copy link
Contributor

@RobeDevOps Yes I think, let's proceed with that approach, this will keep it cleaner and make it easier to debug, thanks!

@RobeDevOps
Copy link
Contributor Author

@dsingh07 I removed the python script and added the python code as simple extension into the bash script file.

  • If the list is empty return empty token so, token is gonna be generated
  • Looping the token arrays and it returns the token ( if exists)
  • Finally returns empty value if not token found.

Please let me know if that is ok. I think the python script is simple and adding jq to the Dockerfile is going to increase the image size and also a new tool and dependencies.

@dsingh07
Copy link
Contributor

Thanks a lot @RobeDevOps that sounds good to me, I'll try and test it as soon as possible and hopefully merge :)

@RobertNorthard
Copy link
Contributor

Thanks @RobeDevOps - I like the approach

dsingh07
dsingh07 previously approved these changes Aug 17, 2018
Copy link
Contributor

@dsingh07 dsingh07 left a comment

Choose a reason for hiding this comment

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

Hi @RobeDevOps LGTM, I have tested this and this works as expected!

  • Image builds fine from the PR
  • ADOP/C instance stands up fine with this image tag used in the compose file
  • Sonar integration in place as expected
  • Brought down ADOP/C instance using ./adop compose down
  • Brought it back up again using ./adop compose init
  • ADOP/C stands up fine again and no errors in the Jenkins logs
  • Sonar integration still in place

Happy to merge, do you think it would be possible to squash commits?

@RobeDevOps
Copy link
Contributor Author

I squashed the commits in one. 👍

@dsingh07
Copy link
Contributor

Thanks @RobeDevOps LGTM, merging!

@dsingh07 dsingh07 merged commit cd631ca into Accenture:master Aug 21, 2018
@dsingh07
Copy link
Contributor

@RobeDevOps New 0.3.5 release is created: https://hub.docker.com/r/accenture/adop-jenkins/builds/

Please can you create an accompanying to adop-docker-compose to update the image tag?

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.

3 participants