-
Notifications
You must be signed in to change notification settings - Fork 8
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
Feature/fix maven and redis and okta checks #456
base: develop
Are you sure you want to change the base?
Feature/fix maven and redis and okta checks #456
Conversation
… This fix is slightly incomplete, the data is still in Redis, it just will stop querying for more. Current. the existing data cannot be deleted.
…s not updated correctly.
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. |
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? |
You are right, it is the library file
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? |
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. |
@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? |
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 osmt_apitest.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) 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? |
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. |
I agree, there should be no file validation, but the log sure seems to say there is. So, that leaves a couple possibilities:
If it's #1, I have no idea what to do next. So, are we talking #1 or #2? |
At great cost to my sanity, here's how to see what the issue is: `--- a/test/bin/pre_test_setup.sh 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. |
…, because it's broken for recent versions, and doesn't work anyways.
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. |
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: |
This is the line that's missing, right, because I see it in my output. So it's there, no matter what git says...
|
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 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. |
The intention here is to add your personal Okta user ID / password / auth endpoint in the env file. 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 [email protected] 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. |
Here's the little nits I've found over the past few days. Nothing incredible earth shattering, but nice to haves.