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

Convert integration tests to use configs.v2 #990

Closed
2 tasks done
sgnn7 opened this issue Nov 8, 2019 · 8 comments · Fixed by #1120
Closed
2 tasks done

Convert integration tests to use configs.v2 #990

sgnn7 opened this issue Nov 8, 2019 · 8 comments · Fixed by #1120

Comments

@sgnn7
Copy link
Contributor

sgnn7 commented Nov 8, 2019

Currently our integration tests for PG and MySQL create generated configuration files for secretless that are designed for config.v1 which has in at least one case caused unexpected problems in the product. These config generators need to be converted to config.v2.

AC:

  • Broker's PG integration tests use config.v2 configs
  • Broker's MySQL integration tests use config.v2 configs
@vladdoster
Copy link

Hi @sgnn7, is this still open for a first time contributor? I enjoyed a CyberArk article on this piece of software and thought I'd contribute.

@sgnn7
Copy link
Contributor Author

sgnn7 commented Nov 13, 2019

@vladdoster Absolutely and we would love to have more contributors to the codebase! Just make sure to read through the contribution doc first. There's a contributor agreement that you can just send us which is somewhat of a pain but you only need to do this once.

@vladdoster
Copy link

vladdoster commented Nov 14, 2019

Hi @sgnn7,

You'll have to bear with me as I am new to Go. I just have a few questions before I proceed further.

So I was able to track down where the MySql and PG created there configs. It seemed to be in generate_secretless_yml.go. In my branch I have added code which seems to take the generated v1 config and converts it to the v2 structure. I have commented it out because it seemed to root of ./bin/test erroring. However, when I run the ./bin/test with my v2 conversion solution, it seems to write the correct structure. See attached screenshot.

pic-selected-191113-2233-27

However it seems to error in MySQL test. Do I need to update the unit tests to take the updated config or is the way I tried converting the v1 config way off? Would love to work through this and fix the issue 😄.

btw - The shell scripts you all have written are top notch 👍

@sgnn7
Copy link
Contributor Author

sgnn7 commented Nov 14, 2019

Hey @vladdoster!

You'll have to bear with me as I am new to Go. I just have a few questions before I proceed further

Don't worry about asking questions and you're on the right track!

The optimal choice here is to not use v1 configuration at all in the configuration generation if possible since v1 will be removed in one of the future releases so depending on it will make us need to rework this code one more time later. To do the conversion, we might require some changes to the generation code so instead of it creating v1 structs we will now be creating v2 structs from the start.

Do I need to update the unit tests to take the updated config

I think that unit tests should not be failing if you change this but integration tests for mysql and pg might since those are the ones that depend on this functionality.

Would love to work through this and fix the issue 😄.

We will help you out whichever way we can!

btw - The shell scripts you all have written are top notch 👍

<3

@vladdoster
Copy link

vladdoster commented Nov 14, 2019

@sgnn7,

I agree my solution was terse because I wasn't sure how much backwards compatibility was an issue. I did see in one of the scripts stdout that v1 would be deprecated so I guess I should have taken the hint 😆. Okay, no worries. I will work on implementing v2 config and get rid of v1 cruft.

Thanks for the feedback.

@izgeri
Copy link
Contributor

izgeri commented Nov 16, 2019

@vladdoster can you also make sure to sign your commits by running git commit with the -s or --signoff flag? We're going to be migrating to the DCO for contributions soon.

@vladdoster
Copy link

@vladdoster can you also make sure to sign your commits by running git commit with the -s or --signoff flag? We're going to be migrating to the DCO for contributions soon.

I can do that. Sorry about glancing over that in my I initial commit.

@izgeri
Copy link
Contributor

izgeri commented Nov 16, 2019

I don't think there's anything publicly published about it yet, so you couldn't have known 🙂 I'm hoping to get it updated before you submit your contribution so that it'll be easier for you.

If you need any help with it or have any questions, please let us know!

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