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

Add env check job definition #66

Merged
merged 1 commit into from
Apr 16, 2024

Conversation

arp-est
Copy link
Contributor

@arp-est arp-est commented Mar 14, 2024

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

@arp-est arp-est self-assigned this Mar 14, 2024
This job checks whether the env variables defined in the cmd-*,
and the envs described in the README match.

Signed-off-by: Arpad Kiss <[email protected]>
Copy link
Member

@denis-tingaikin denis-tingaikin left a 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


}

Comment on lines +29 to +36
build_cmd() {
local -r repo="$1"
local -r output="$2"
pushd . &>/dev/null
cd "$repo"
go build -o "$output" .
popd &>/dev/null
}
Copy link
Member

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

Copy link
Contributor Author

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)
Copy link
Member

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?

Copy link
Contributor Author

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.

@arp-est
Copy link
Contributor Author

arp-est commented Mar 19, 2024

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


}

Thanks for the comments,
The unit_test approach does look a lot simpler, I'll check it out :)

@arp-est
Copy link
Contributor Author

arp-est commented Apr 3, 2024

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.

  1. Not every config is defined in the same file on the cmds, some are in
    internal/config.go, some are in the main.go, and to use the config struct, I would need to know it at compile time.

  2. the "prefix" of the config variables, eg.: 'NSM in NSM_NAME' is not always prefixed
    with NSM, but in the 'cmd-registry-memory' repo it uses 'REGISTRY_MEMORY' as a prefix
    and since this is hardcoded into the code eg.: 'envconfig.Usage("nsm", c)' there's no go only approach to get it.

to solve these, I was playing around with writing not a test, but a template, and
the ci would fill out the parameters depending on what's in the cmd repo, generating a test file.

At present I have these two files, one a template, and a script, that extracts
the module and prefix from the repo, and generates a test out of it.
asd.tar.gz

If you put these next to each other, and call the 'unpack_template.sh' on a
repo, it should add the test into the repo, and can call it from inside as 'go
test' should work.

Currently I'm checking out how the cmd-template repo works, my thought is that
if I could make the bot to call the 'unpack_template.sh' and generate a
different env_test.go for each cmd, that would be nice, but not sure if that's
easily achievable.

A slightly different approach could be if the ci to just downloads these two
files from a central place, and generates an env_test.go for itself, and just
runs it.

Let me know your opinion if these thoughts look good, or if it complicates things a bit too much. (:

@denis-tingaikin
Copy link
Member

@arp-est
Copy link
Contributor Author

arp-est commented Apr 4, 2024

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.
So I'm on the opinion to just go with the current bash script if that's acceptable. (I think I can improve it a bit though, by grep-ing out the prefix from the repo, that would remove the need to provide the parameter to the job)

Copy link
Member

@denis-tingaikin denis-tingaikin left a 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?

@denis-tingaikin
Copy link
Member

@arp-est Merging... Could we test this workflow in https://github.com/networkservicemesh/cmd-nsc?

@denis-tingaikin denis-tingaikin merged commit 382f5e7 into networkservicemesh:main Apr 16, 2024
2 checks passed
@arp-est
Copy link
Contributor Author

arp-est commented Apr 16, 2024

@arp-est Merging... Could we test this workflow in https://github.com/networkservicemesh/cmd-nsc?

Sure, I'll prepare a merge request

@arp-est
Copy link
Contributor Author

arp-est commented Apr 16, 2024

@denis-tingaikin
Looks promising,
Made a draft here: networkservicemesh/cmd-nsc#625, and
the last runs look good.
Without changing the Readme it failed as it should: https://github.com/networkservicemesh/cmd-nsc/actions/runs/8707315199
and when I changed it it succeeded: https://github.com/networkservicemesh/cmd-nsc/actions/runs/8707599728

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