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 label to extension analyze API #1132

Merged
merged 9 commits into from
Jun 27, 2023
Merged

Add label to extension analyze API #1132

merged 9 commits into from
Jun 27, 2023

Conversation

cd-work
Copy link
Contributor

@cd-work cd-work commented Jun 22, 2023

This adds the new label option as the last optional parameter of the analyze function, allowing phylum-ci to use the extension API for its analysis.

Closes #1131.

This adds the new `label` option as the last optional parameter of the
`analyze` function, allowing `phylum-ci` to use the extension API for
its analysis.
@cd-work cd-work requested a review from a team as a code owner June 22, 2023 21:33
@cd-work cd-work requested a review from maxrake June 22, 2023 21:33
@cd-work
Copy link
Contributor Author

cd-work commented Jun 22, 2023

I plan to remove the extension itself before merging this, since it's probably better to have it in the phylum-ci repository. However this should make testing easier.

Copy link
Contributor

@maxrake maxrake left a comment

Choose a reason for hiding this comment

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

Local testing has not been performed yet...but it will be if/when the suggested changes are made.

extensions/phylum.ts Show resolved Hide resolved
extensions/ci/main.ts Outdated Show resolved Hide resolved
extensions/ci/main.ts Outdated Show resolved Hide resolved
@cd-work cd-work requested a review from maxrake June 22, 2023 22:18
maxrake
maxrake previously approved these changes Jun 26, 2023
Copy link
Contributor

@maxrake maxrake left a comment

Choose a reason for hiding this comment

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

The changes were tested locally and found to work with phylum-ci after a few changes were made in that repo. The real trick is making a ci extension available in the phylum-ci Docker image.

The current method for making phylum available in the image relies on the custom/hidden --global-install option. Something similar may have to be done for this extension since the expectation is that using phylum extension install will result in an extension installed in the root user's directory and then not be available to non-root users.

Perhaps the extension could be copied manually to a globally accessible directory (like the binary is) and then accessed with a phylum extension run command instead of a phylum ci command.

extensions/ci/main.ts Outdated Show resolved Hide resolved
@cd-work
Copy link
Contributor Author

cd-work commented Jun 27, 2023

Something similar may have to be done for this extension since the expectation is that using phylum extension install will result in an extension installed in the root user's directory and then not be available to non-root users.

Can't you just install it as root?

@maxrake
Copy link
Contributor

maxrake commented Jun 27, 2023

Can't you just install it as root?

Maybe...if the permissions can also be changed to allow access by non-root users. There are some environments...like Azure Pipelines...where a container is created from the image with a non-root user. From previous issue covering this:

This is a problem for some CI environments. For instance, Azure Pipelines creates containers from a given image with a user named vsts_azpcontainer and an id of 1001, in a group named azure_pipelines_sudo. Tasks/scripts/commands run from that container are done so with this user, which doesn't have access to the /root/.local/bin directory where the phylum Python package script entry points are located.

maxrake
maxrake previously approved these changes Jun 27, 2023
Copy link
Contributor

@maxrake maxrake left a comment

Choose a reason for hiding this comment

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

The example ci extension has been tested and shown to work in the phylum-ci repo. It can be removed from this PR now if that is still the plan. The rest of the code changes in this PR are desired in a release candidate for use in a new phylum-ci release.

@cd-work
Copy link
Contributor Author

cd-work commented Jun 27, 2023

I've moved the extension to phylum-dev/phylum-ci#267 for now.

Copy link
Contributor

@maxrake maxrake left a comment

Choose a reason for hiding this comment

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

LGTM.

@cd-work cd-work merged commit c414970 into main Jun 27, 2023
8 checks passed
@cd-work cd-work deleted the phylum_ci_ext branch June 27, 2023 19:01
kylewillmon pushed a commit that referenced this pull request Jun 28, 2023
This adds the new `label` option as the last optional parameter of the
`analyze` function, allowing `phylum-ci` to use the extension API for
its analysis.
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.

analyze command output breaks phylum-ci
2 participants