-
Notifications
You must be signed in to change notification settings - Fork 12
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
Add env check job definition #66
Add env check job definition #66
Conversation
This job checks whether the env variables defined in the cmd-*, and the envs described in the README match. Signed-off-by: Arpad Kiss <arpad.a.kiss@est.tech>
1e85b27
to
49e466c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also a bit discussed this PR locally, and probably creating a unit test that checks the env configuration for each repository would be a more simple approach. Because when we merge the PR, it will also require an update in each repository to fix the issues.
func TestReadme(t *testing.T) {
varc config.Config
varexpectedBuilder strings.Builder
envconfig.Usagef("NSM", c, &expectedBuilder, "")
b, err:=os.ReadFile("README.md")
require.NoError(t, err)
varactual=string(b) //TODO: parse envs from the readme
//TODO: check that expectedBuilder contains all envs from the actual
}
build_cmd() { | ||
local -r repo="$1" | ||
local -r output="$2" | ||
pushd . &>/dev/null | ||
cd "$repo" | ||
go build -o "$output" . | ||
popd &>/dev/null | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be done as a step of the github job
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, my though here was that it would be more convenient to have the script do the build, and that way you have a contained script that can be called on any repo manually without the job, but I don't really have any strong feelings about it.
I guess in the unit_test approach its a bit cleaner to have this in the job itself.
build_cmd "$repo" "$cmd_name" | ||
declare -a cmd_envs | ||
# Since this runs on "ubuntu-runner" I can be sure that no env vars are set and cmd fails | ||
mapfile -t cmd_envs < <( "${repo}/${cmd_name}" 2>&1 | grep -Eo "${env_prefix}\w+" | uniq | sort) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will the cmd application run here forever?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I've tested the script on every cmd, and each one of them has terminated. It is probably not the most future proof approach though.
Anyway, I think that in the unit_test approach this will not be an issue, as that does not run the cmd itself only includes the config, I assume.
Thanks for the comments, |
Hi, I looked into the test case approach, and I have a few problems that makes it a bit more difficult than I would like.
to solve these, I was playing around with writing not a test, but a template, and At present I have these two files, one a template, and a script, that extracts If you put these next to each other, and call the 'unpack_template.sh' on a Currently I'm checking out how the cmd-template repo works, my thought is that A slightly different approach could be if the ci to just downloads these two Let me know your opinion if these thoughts look good, or if it complicates things a bit too much. (: |
Just found that at places the Config definition, and envconfig.Usage call is in different files :'D, so its more difficult to grep out the which module has the Config. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can go ahead and test it with one of our repos.
@NikitaSkrynnik Could you please have a look into the PR?
@arp-est Merging... Could we test this workflow in https://github.com/networkservicemesh/cmd-nsc? |
Sure, I'll prepare a merge request |
@denis-tingaikin |
This job checks whether the env variables defined in the cmd-*, and the envs described in the README match.
Issue: #65
Description:
So basically I made script that for now greps out the envs from cmd, and README, and compares whether these match
There was one repo (cmd-registry-memory) where the prefix was not NSM_, so I made it configurable.
I'm a bit unsure whether the bash script itself is in a good place here, so if you guys know a better place please tell me :)
Usage wise this is only the job, so it should be called from the specific cmd's workflow
There are a few example executions here on Nordix: https://github.com/Nordix/nsm-cmd-forwarder-sriov/actions