wgu-opensource / osmt

OSMT is the Open Skills Management Tool
https://osmt.io
Other
44 stars 8 forks source link

Feature/fix maven and redis and okta checks #456

Open duckhead opened 11 months ago

duckhead commented 11 months ago

Here's the little nits I've found over the past few days. Nothing incredible earth shattering, but nice to haves.

duckhead commented 11 months ago

I assume it's normal for the tests to fail. Runs fine from where I sit, which is on a Debian bookworm box.

JohnKallies commented 11 months ago

I assume it's normal for the tests to fail. Runs fine from where I sit, which is on a Debian bookworm box.

It's not normal to fail. The issue reported seems to be around the grep for the API test example file as well. Let's look closer at the GH Actions log? I'm looking as well.

JohnKallies commented 11 months ago

I assume it's normal for the tests to fail. Runs fine from where I sit, which is on a Debian bookworm box.

Try adding this at line 81 in osmt_cli.sh

echo_info "Okta values are not provided by environment variables. Sourcing ${env_file} env file."

duckhead commented 11 months ago

I assume it's normal for the tests to fail. Runs fine from where I sit, which is on a Debian bookworm box.

It's not normal to fail. The issue reported seems to be around the grep for the API test example file as well. Let's look closer at the GH Actions log? I'm looking as well. That won't wor

I assume it's normal for the tests to fail. Runs fine from where I sit, which is on a Debian bookworm box.

Try adding this at line 81 in osmt_cli.sh

echo_info "Okta values are not provided by environment variables. Sourcing ${env_file} env file."

That won't work. First, I think it's common.sh you want me to change. This change will basically bypass reading the editable file (osmt_apitest.env) if we supplied env variables. Since the test runner isn't supplying those, we will attempt to source it. That file doesn't exist, because as far as I know, osmt_cli.sh -i hasn't run. Are you saying we should load the osmt_apitest.env.example file it the osmt_apitest.env file doesn't exist?

JohnKallies commented 11 months ago

That won't work. First, I think it's common.sh you want me to change.

You are right, it is the library file

This change will basically bypass reading the editable file (osmt_apitest.env) if we supplied env variables. Since the test runner isn't supplying those, we will attempt to source it. That file doesn't exist, because as far as I know, osmt_cli.sh -i hasn't run. Are you saying we should load the osmt_apitest.env.example file it the osmt_apitest.env file doesn't exist?

No, these values should be provided by the runner. It looks like they aren't , so I was looking for log output that would confirm that.

duckhead commented 11 months ago

That won't work. First, I think it's common.sh you want me to change.

You are right, it is the library file

This change will basically bypass reading the editable file (osmt_apitest.env) if we supplied env variables. Since the test runner isn't supplying those, we will attempt to source it. That file doesn't exist, because as far as I know, osmt_cli.sh -i hasn't run. Are you saying we should load the osmt_apitest.env.example file it the osmt_apitest.env file doesn't exist?

No, these values should be provided by the runner. It looks like they aren't , so I was looking for log output that would confirm that.

OK, reverting test/osmt-apitest.env.example and rerunning osmt_cli.sh -i fixes the issue. So, it is my change. The mvn verify... makes it much further with the file reverted.

I don't know much more yet, I've never tried tracing into a maven command before. But, i suspect that we are failing because we are now seeing XXXXXX. So, some kind of validation is failing, regardless of specified environment variables or command lines. The solution should be to not do that validation if we have the inputs otherwise, just like we won't source the .env file we have the inputs otherwise. I don't know where that is yet though.

For my own edification, why use username/password here, and the OpenID stuff in api/ is that just some work that wasn't finished?

duckhead commented 11 months ago

OK, I have no idea how to trace down what Maven is doing. But, here's my suspicion. At some point, we perform the same checks that osmt_cli.sh -v does. We see that we have xxx in the .env file, and exit out.

Before, the values were XXX and always passed. Now they are not, and always fail. The fix, from where I set, would be to do the same check that is done in that code you pointed me to earlier (line 81 in common.sh, the 3 -n checks) and add that into _validate_env_fille. Or, maybe at the caller instead, where we knew we are running the API tests.

I'll try and play with that tomorrow and see what I get, If that fixes it, cool. If not, I'm going to need to know what code checks the config and decides it's not worthy.

JohnKallies commented 11 months ago

@duckhead it's failing before any validation, on the test for "file exists" and "can read the file". I double checked that the secrets OKTA_ still exist in the repo, they are good.

I added a PR for the additional logging I mentioned, and added you as a reviewer. Would you please give it a thumb up?

JohnKallies commented 11 months ago

Maven is simply calling the pre_test_setup.sh script, which sources the common functions, declares some of its own functions, and then calls main. The first thing it does is source osmtapitest.env, if the 3 OKTA vars don't exist. I checked them, and they are still being provided to the runner. I really don't see how your change could impact that. Still looking at it...

duckhead commented 11 months ago

Maven is simply calling the pre_test_setup.sh script, which sources the common functions, declares some of its own functions, and then calls main. The first thing it does is source osmtapitest.env, if the 3 OKTA vars don't exist. I checked them, and they are still being provided to the runner. I really don't see how your change could impact that. Still looking at it...

I do understand what's going on, and I think my analysis above is correct. I'm just not expressing it well. I think it's a little easier with your one-liner though, so I'll try again. This is what I'm getting after I applied the one line change:

INFO: OAUTH values are provided by environment variables. Not sourcing /home/jfrank/osmt/api/osmt-dev-stack.env env file.

INFO: Checking /home/jfrank/osmt/api/osmt-dev-stack.env for invalid default values (xxxxxx) INFO: No invalid default values in /home/jfrank/osmt/api/osmt-dev-stack.env INFO: Sourcing /home/jfrank/osmt/api/osmt-dev-stack.env

So, your new one liner is called first, and it did the right thing. It found we had the env variables, and didn't source the file. The next line is only found in common.sh, in _validate_env_file. So, we are calling that. That is only called from osmt_cli.sh -v, in this line:

/osmt_cli.sh:42: _validate_env_file "${APITEST_ENV_FILE}" || is_environment_valid+=1

The issue is that we need to do a -n on the 3 OKTA variables here, and if those are set, skip this line, but still add one to is_environment_valid. Does that make any more sense?

JohnKallies commented 11 months ago

Unless I'm missing it, there shouldn't be any env file validation or dependency in a GH Action workflow. They are not created, because the secrets are provided by the runner.

duckhead commented 11 months ago

I agree, there should be no file validation, but the log sure seems to say there is. So, that leaves a couple possibilities:

  1. It's not validating the files, and I can't read logs.
  2. It should not be validating files, but it is. And that needs to be fixed.

If it's #1, I have no idea what to do next. If it's #2, I know a fix (fix the validation code to skip validation if we have the env variables), but it sounds like you're favoring a different fix (don't validate anything (or maybe just not the files)) In that case, maybe add a --skip-file-validation to osmt_cli.sh seems like the easiest fix.

So, are we talking #1 or #2?

duckhead commented 11 months ago

At great cost to my sanity, here's how to see what the issue is:

`--- a/test/bin/pre_test_setup.sh +++ b/test/bin/pre_test_setup.sh @@ -37,6 +37,7 @@ error_handler() {

main() {

Sourcing API test env file

I have no idea why that's not accepting the code markdown. I give up...

If you add that and run the test, you will find that these variables are not set. I don't know what sets those, so I'll have to leave that up to you guys.

JohnKallies commented 11 months ago

@duckhead please rebase from develop to pick up #457. If we still a problem with your changes, let's back out the XXX lower case fix and get the benefit of your other 2 changes?

duckhead commented 11 months ago

@duckhead please rebase from develop to pick up #457. If we still a problem with your changes, let's back out the XXX lower case fix and get the benefit of your other 2 changes?

OK, I've rebased and rolled back the XXX change. I also put in the docker version fix, and the echo_info I used to find that the variables were missing, but that is commented out for now, since it still needs addressing.

JohnKallies commented 11 months ago

I don't see where you've rebased my changes from develop in yet.

image

I don't think my changes will save the day, but I'm trying to run down how changes from a committer vs a maintainer affects how GH Runners consume secrets. I'd like to see those log entries in context in your build output. Would you please rebase from develop when you get a moment? Thank you

duckhead commented 11 months ago

I don't see where you've rebased my changes from develop in yet. image

I don't think my changes will save the day, but I'm trying to run down how changes from a committer vs a maintainer affects how GH Runners consume secrets. I'd like to see those log entries in context in your build output. Would you please rebase from develop when you get a moment? Thank you

I'm a command-line guy, that way I can easily keep track of what I've messed up. I did this today, but I did the same yesterday.

jfrank@yellow:~/osmt$ git rebase --onto develop Successfully rebased and updated refs/heads/feature/fixMavenAndRedisAndOktaChecks. frank@yellow:~/osmt$ git pull Updating c3a80f91..900ad76f Fast-forward bin/lib/common.sh | 2 -- docker/base-images/Dockerfile.osmt-build | 2 +- docker/dev-stack.yml | 1 + test/bin/pre_test_setup.sh | 1 + test/osmt-apitest.env.example | 4 ++-- 5 files changed, 5 insertions(+), 5 deletions(-) jfrank@yellow:~/osmt$ git push Everything up-to-date

duckhead commented 11 months ago

This is the line that's missing, right, because I see it in my output. So it's there, no matter what git says...

INFO: OAUTH values are provided by environment variables. Not sourcing /home/jfrank/osmt/api/osmt-dev-stack.env env

JohnKallies commented 11 months ago

Hang on.... I've been looking at your log output in the GH Actions. Are you having these failures locally? You should not be having issues locally if you have created your local osmt-apitest.env file and provided the proper Okta values there.

Please clarify?

duckhead commented 11 months ago

Hang on.... I've been looking at your log output in the GH Actions. Are you having these failures locally? You should not be having issues locally if you have created your local osmt-apitest.env file and provided the proper Okta values there.

Please clarify?

I am having these issues locally. It's because unlike in the API tests, you want username/password here. And, I don't have, nor can I find, a username/password anywhere in OKTA. That was why I asked why we did it this way above somewhere.

JohnKallies commented 11 months ago

The intention here is to add your personal Okta user ID / password / auth endpoint in the env file. image

Would you please confirm that you have this locally and how your API test Maven profile execution completes?

Also, let's discuss more directly? Email me at john.kallies@wgu.edu if you are willing? I don't (can't) do this for community support, but we are starting to talk about auth issues and it might be easier / safer off GitHub.