Skip to content

Adds validation for required keys in endpoint and request nodes#147

Merged
gillianomenezes merged 2 commits intoscanapi:masterfrom
gillianomenezes:validate-required-keys
May 18, 2020
Merged

Adds validation for required keys in endpoint and request nodes#147
gillianomenezes merged 2 commits intoscanapi:masterfrom
gillianomenezes:validate-required-keys

Conversation

@gillianomenezes
Copy link
Copy Markdown
Contributor

Adds required keys to request and endpoint nodes.

@camilamaia camilamaia requested review from barbosa and camilamaia May 18, 2020 11:40
@camilamaia
Copy link
Copy Markdown
Member

camilamaia commented May 18, 2020

Hmmm the Changelog Reminder job is failing because it has no permissions, since it is a fork. Also, the other checks are not running, like the one that run the tests.

@barbosa do you know how other projects handle forks and CI automations like this one? I am going to investigate it.

@camilamaia
Copy link
Copy Markdown
Member

Closes #64

Copy link
Copy Markdown
Member

@camilamaia camilamaia left a comment

Choose a reason for hiding this comment

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

Really great PR 🤩 Just one minor suggestion

Comment thread scanapi/utils.py


def validate_required_keys(keys, required_keys, scope):
if not set(required_keys) <= set(keys):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@gillianomenezes ahá! You are using sets ❤️ Perfect use case!

Comment thread scanapi/utils.py Outdated

def validate_required_keys(keys, required_keys, scope):
if not set(required_keys) <= set(keys):
missing_keys = list(set(required_keys) - set(keys))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
missing_keys = list(set(required_keys) - set(keys))
missing_keys = set(required_keys) - set(keys)

What do you think of send a set instance to MissingMandatoryKeyError init instead of a list, avoiding this casting?

", ".join(sorted(missing_keys)) works fine for sets too!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I didn't know that. I'll update. Thank you!

@barbosa
Copy link
Copy Markdown
Member

barbosa commented May 18, 2020

Hmmm the Changelog Reminder job is failing because it has no permissions, since it is a fork. Also, the other checks are not running, like the one that run the tests.

@barbosa do you know how other projects handle forks and CI automations like this one? I am going to investigate it.

@camilamaia apparently this is a known issue: peterjgrainger/action-changelog-reminder#28

Not from the action itself but a GitHub Action's limitation at the moment. The best we can do right now is to simply ignore it for fork PRs until this is fixed.

        uses: peterjgrainger/action-changelog-reminder@v1.2.0
+       if: github.repository == 'scanapi/scanapi'
        with:
          changelog_regex: "CHANGELOG.md"
        env:
          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}

more: actions/labeler#12

@camilamaia camilamaia self-requested a review May 18, 2020 19:56
@gillianomenezes gillianomenezes merged commit 1fa5290 into scanapi:master May 18, 2020
@barbosa
Copy link
Copy Markdown
Member

barbosa commented May 27, 2020

@camilamaia I have just bumped into this:

Workflows don't run on forked repositories by default. You must enable GitHub Actions in the Actions tab of the forked repository.

https://help.github.com/en/actions/reference/events-that-trigger-workflows

@camilamaia
Copy link
Copy Markdown
Member

@camilamaia I have just bumped into this:

Workflows don't run on forked repositories by default. You must enable GitHub Actions in the Actions tab of the forked repository.

https://help.github.com/en/actions/reference/events-that-trigger-workflows

Wow, this is super tricky. Thanks for bringing it. Next PR from a fork we can ask the person to enable it then... This was the only thing i could come up with

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