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

Fix environment variable processing. (#406) #407

Merged
merged 8 commits into from
May 10, 2023
Merged

Conversation

radolin
Copy link
Contributor

@radolin radolin commented Mar 9, 2023

Search for static and non public members as well when overriding values. Here's how the member is defined in GitHubHealthAlgorithm.cs:
private static string? ENV_GITHUB_ACCESS_TOKEN = null;

Searching for public members only misses it and it's never set from the environment variable.
I don't know if this is the right way to fix this, but at least it get's me one step further and I can actually run the oss-health tool for an npm project.

Search for static and non public members as well when overriding values.
@gfs
Copy link
Contributor

gfs commented Mar 9, 2023

Hi @radolin,

Thanks for the report and proposed fix. I'll have to think about this a bit. It might be better just to make the GitHub token field public and non-static - I don't think there's a particular reason for it to be private - or static - but overriding private vars in other circumstances might not be something we want.

@gfs
Copy link
Contributor

gfs commented Mar 9, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@gfs
Copy link
Contributor

gfs commented Mar 9, 2023

This report revealed a number of environment variables with the incorrect settings (private or static). I went through and instead of modifying environment helper, changed all properties we intend to be overridden by Environment Helper to be Public instance variables.

@gfs gfs requested a review from jpinz March 9, 2023 17:13
@gfs
Copy link
Contributor

gfs commented Mar 9, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@radolin
Copy link
Contributor Author

radolin commented Mar 9, 2023

After your changes I was back to always getting the "Missing GitHub access token. Please define GITHUB_ACCESS_TOKEN." error. I've added 2 commits making ENV_GITHUB_ACCESS_TOKEN a property and changed EnvironmentHelper to look for properties instead of fields. Now it can process the GITHUB_ACCESS_TOKEN environment variable again.

@radolin
Copy link
Contributor Author

radolin commented Mar 9, 2023

/azp run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 407 in repo microsoft/OSSGadget

@jpinz
Copy link
Member

jpinz commented Mar 9, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@gfs
Copy link
Contributor

gfs commented Mar 9, 2023

Thanks @radolin for catching the needed change to grab properties.

@gfs
Copy link
Contributor

gfs commented Mar 9, 2023

@radolin one more thing, can you reply to the Contributor agreement bot? I can't accept the contribution otherwise.

@jpinz any comments/concerns here?

@radolin
Copy link
Contributor Author

radolin commented Mar 10, 2023

@radolin one more thing, can you reply to the Contributor agreement bot? I can't accept the contribution otherwise.

Will, do, just need to get the blessing from legal for a new CLA. Might take a couple of days.

Copy link
Member

@jpinz jpinz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming it works and everything, LGTM

@gfs
Copy link
Contributor

gfs commented Apr 24, 2023

@radolin any update from your legal team?

@radolin
Copy link
Contributor Author

radolin commented May 5, 2023

@microsoft-github-policy-service agree company="SUSE"

@radolin
Copy link
Contributor Author

radolin commented May 10, 2023

All checks seem green now, sorry for the delay.

@gfs gfs merged commit 654963b into microsoft:main May 10, 2023
1 check passed
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.

None yet

3 participants