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

V2.2.0 #47

Open
wants to merge 50 commits into
base: master
Choose a base branch
from
Open

Conversation

MarlonHeiber
Copy link
Contributor

No description provided.

@MarlonHeiber MarlonHeiber marked this pull request as ready for review July 19, 2022 20:08
@MarlonHeiber
Copy link
Contributor Author

@nzlosh @armab All changes were put here in only one PR. The modifications that are in others PRs that are closed now are here. If you could make a review please. Thanks.

Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

What is the purpose of 7788189 ?
I believe CI clones the lint-configs repo, so I'm concerned this will get out of date. Could we remove lint-configs/?

Copy link

@amanda11 amanda11 left a comment

Choose a reason for hiding this comment

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

I've requested some changes on the yaml files, not had chance to check code in detail - but noticed some discrepancies on the yaml files.
We could move to using python3 f-strings to simplify formatting of the strings rather than using .format etc, but that's just a suggestion - something we can now use now that we've dropped python2 support.

CHANGES.md Outdated Show resolved Hide resolved
default: "{{action_context.api_user|default(None)}}"
owner:
type: "string"
description: "The account owner of the repository. The name is not case sensitive.."

Choose a reason for hiding this comment

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

Suggested change
description: "The account owner of the repository. The name is not case sensitive.."
description: "The account owner of the repository. The name is not case sensitive."

# Repository parameters
owner:
type: "string"
description: "The account owner of the repository. The name is not case sensitive.."

Choose a reason for hiding this comment

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

Suggested change
description: "The account owner of the repository. The name is not case sensitive.."
description: "The account owner of the repository. The name is not case sensitive."

description: >
Add or update repository team.
Example:
st2 run github.add_update_repository_team organization="organization" owner="owner" repo="reponame" team_slug="team_id" api_user="token_name"

Choose a reason for hiding this comment

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

This example uses a parameter called organization, but the yaml states that the parameter is called org.

description: >
Add or update a repository environment.
Example:
st2 run github.add_update_repository_environment organization="organization" owner="owner" repo="reponame" reviewers="< array of reviewers >" api_user="token_name"

Choose a reason for hiding this comment

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

I can't see a parameter organization in the yaml. It looks like in the python code, that owner is being passed as organisation to the get_team_id - so that parameters in the example and the yaml don't seem to match.
Also yaml says environment is required parameter with no default, yet its not used in the example.

Choose a reason for hiding this comment

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

thanks! I have rewritten this and tested the call, it seems to be okay now :)

still pending to push the code though

github_type:
type: "string"
description: "The type of github installation to target, if unset will use the configured default."
default: ~

Choose a reason for hiding this comment

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

Why is default "~" - should this have been one of enterprise or online?

Choose a reason for hiding this comment

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

that's a good question! it's been like that since before.. I have pushed a commit to align all github_type usage and doc.

it is not required, and it can only be enterprise/online.. the default being whatever is configured in the pack, which is online by default

@@ -32,3 +32,15 @@ parameters:
type: "string"
description: "The name of the branch you want the changes pulled into. This should be an existing branch on the current repository. You cannot submit a pull request to one repository that requests a merge to a base of another repository."
required: true
api_user:
type: "string"
description: "The"

Choose a reason for hiding this comment

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

I think the description for api_user hasn't been finished, as its just "The"

github_type:
type: "string"
description: "The type of github installation to target, if unset will use the configured default."
default: ~

Choose a reason for hiding this comment

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

Why is default for github_type ~ rather than one of enterprise or online.

@@ -49,3 +49,14 @@ parameters:
type: "string"
description: "If omitted this will be filled in with committer information. If passed, you must specify both a name and email. Expected format: FirstName LastName <email@address>"
required: false
api_user:
type: "string"
description: "The"

Choose a reason for hiding this comment

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

Description for api_user not complete, as its just "The"

github_type:
type: "string"
description: "The type of github installation to target, if unset will use the configured default."
default: ~

Choose a reason for hiding this comment

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

Why is default ~ rather than enterprise or online?

@pimguilherme
Copy link

hi @amanda11 ! thanks a lot for validating all the changes :)

i have gone through all of them and fixed them to my best knowledge.. also tried to run all example cases within the action description and they all seem fine now!

I would like to add test cases for the actions but I guess that could be tackled in another moment

cheers!

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.

4 participants