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

Gracefully Handle Missing Permissions #11

Merged
merged 4 commits into from
Aug 28, 2020

Conversation

mcdonnnj
Copy link
Member

πŸ—£ Description

This PR adds functionality to gracefully handle a previously fatal case of missing permissions. This should resolve #4 . It also adds a status badge to the README for the lineage_scan workflow.

πŸ’­ Motivation and Context

We have run into the issue where a repository does not have the correct permissions configured, and this causes the Lineage workflow to fail. If unnoticed, it can go on for an extended period of time (the latest was four weeks). When this happens, the workflow will only process repositories until it hits the problem repository. Anything in the search results after that will not be updated.

The new badge allows an at-a-glance check for the last lineage_scan workflow run.

πŸ§ͺ Testing

Automated tests pass.

πŸš₯ Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (causes existing functionality to change)

βœ… Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@mcdonnnj mcdonnnj requested a review from a team as a code owner August 27, 2020 18:26
@mcdonnnj mcdonnnj self-assigned this Aug 27, 2020
src/lineage/entrypoint.py Outdated Show resolved Hide resolved
netloc=f"{username}:{password}@{parts.netloc}"
).geturl()
logging.info("Assigning credentials for push.")
run([GIT, "remote", "set-url", "origin", cred_url], cwd=repo.full_name)
Copy link
Member

Choose a reason for hiding this comment

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

At some point we should replace these git commands with GitPython.

Copy link
Member

Choose a reason for hiding this comment

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

See #12, which I just created.

Copy link
Member Author

Choose a reason for hiding this comment

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

@felddy Do you remember why you didn't leverage that when you first wrote this?

Copy link
Member

Choose a reason for hiding this comment

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

Probably didn't know about it yet. Not a technical reason.

Copy link
Member

@jsf9k jsf9k left a comment

Choose a reason for hiding this comment

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

I'd like those logging.warning("ERROR!") messages to be made consistent. Otherwise this looks good to me.

@mcdonnnj mcdonnnj requested a review from jsf9k August 27, 2020 19:55
Copy link
Member

@felddy felddy left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for fixing my omissions. See my two ideas about warnings.

body = pystache.render(clean_template, template_data)
create_pull_request(
repo, pr_branch_name, local_branch, title, body, draft=False
logging.info(
Copy link
Member

Choose a reason for hiding this comment

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

Should we make this a warning?

Copy link
Member

Choose a reason for hiding this comment

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

Let's make it a warning, but make sure the warning string starts with "ERROR!'

create_pull_request(
repo, pr_branch_name, local_branch, title, body, draft=False
logging.info(
"Not creating a new pull request since the branch already existed."
)
else:
logging.info(
Copy link
Member

Choose a reason for hiding this comment

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

Warning?

Copy link
Member

@jsf9k jsf9k left a comment

Choose a reason for hiding this comment

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

Strong work!

Copy link
Member

@dav3r dav3r left a comment

Choose a reason for hiding this comment

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

Nice fixes in here πŸš€

@mcdonnnj mcdonnnj merged commit 13c4350 into develop Aug 28, 2020
@mcdonnnj mcdonnnj deleted the bugfix/fix_permissions_failure branch August 28, 2020 03:21
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.

Permission errors should not cause job failure
4 participants