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 docker authentication #51

Merged
merged 5 commits into from Nov 2, 2020
Merged

Conversation

dnadeau-lanl
Copy link
Contributor

@dnadeau-lanl dnadeau-lanl commented Oct 28, 2020

Add Docker authentication to images being pulled.

  • Pull request has descriptive title
  • Pull request gives overview of changes
  • [N/A] New code has inline comments where necessary
  • [N/A] Any new modules, functions or classes have docstrings consistent with dbprocessing style
  • [N/A] Major new functionality has appropriate Sphinx documentation
  • [N/A] Added an entry to CHANGELOG if fixing a major bug or providing a major new feature
  • [N/A] New features and bug fixes should have unit tests
  • Relevant issues are linked in the description (e.g. See issue # or Closes #)

Closes #32

@dnadeau-lanl dnadeau-lanl self-assigned this Oct 28, 2020
@dnadeau-lanl dnadeau-lanl added enhancement New feature or feature request testing Affects tests not functionality labels Oct 28, 2020
@dnadeau-lanl dnadeau-lanl added this to In progress in Continuous Integration via automation Oct 28, 2020
@dnadeau-lanl dnadeau-lanl linked an issue Oct 28, 2020 that may be closed by this pull request
@jtniehof
Copy link
Member

The check on this is weird...Github is showing the webhook as delivered to CircleCI, and the response is the same as for #50, but there's no indication of it running on the CircleCI side. I'll keep digging...the difference might be in who submitted, but #41 was fine.

There is a build on CircleCI against master, which is weird.

Ah, but I did have the "require up-to-date before merging" set; I wasn't going to turn that off until I did other work for #40, but I'll switch that now and see if it makes a difference.

Two things that would help

  1. Can you add the "Closes Add Docker auth to CircleCI #32" to the PR description? That's how the release notes process pulls the information; manually linking doesn't show up in there.
  2. Can you add a little information on what has to be set up on the CircleCI side to inject the secrets into the environment? This can go in the CircleCI section of https://github.com/spacepy/dbprocessing/blob/master/docs/developer/github.rst and that gives a little bit of an example of how I had other stuff documented.

@dnadeau-lanl
Copy link
Contributor Author

Will do today. I can try to retrigger circleCI with a "curl" command.

@jtniehof
Copy link
Member

I tried the retrigger and nothing. The problem is we need to enable the "Pass secrets to builds from forked pull requests" setting in CircleCI. I just turned that on and will retrigger; that needs to go in the setup documentation, too.

@jtniehof
Copy link
Member

Incidentally the thing with that setting is that somebody can submit a PR that changes the CircleCI config to just spit your Docker credentials to their email, so don't do anything terribly sensitive with those credentials...

@dnadeau-lanl
Copy link
Contributor Author

I can see in the "response" X-Frame-Options: DENY, hopefully passing secrets will work.

@dnadeau-lanl
Copy link
Contributor Author

I don't think I have access 😏
https://github.com/spacepy/dbprocessing/settings/access

@jtniehof
Copy link
Member

You're a member of the dbprocessing group, so you have access.

X-Frame-Options: DENY is set on all CircleCI replies; it just means "don't stick this in a frame." It's being set on builds that worked, too.

I can't find any logging on the CircleCI side of what hooks it's received and what it did with them.

@jtniehof
Copy link
Member

Ah, at least I figured out the "master" builds. When a PR is merged, that's sending a "push to master" event to CircleCI, which is then triggering a build on master. I'll make a note to disable that as part of #40, since we'll be doing the merge before running the PR tests and also doing cron-based builds of master...no sense in running the tests twice.

Still can't figure out why redelivery of the webhook isn't triggering the pipeline.

@jtniehof
Copy link
Member

Did you just disable the integration checks?

@dnadeau-lanl
Copy link
Contributor Author

dnadeau-lanl commented Oct 29, 2020 via email

@jtniehof jtniehof closed this Oct 29, 2020
Continuous Integration automation moved this from In progress to Done Oct 29, 2020
@jtniehof jtniehof reopened this Oct 29, 2020
Continuous Integration automation moved this from Done to In progress Oct 29, 2020
@dnadeau-lanl
Copy link
Contributor Author

I just reran it within circleci

@dnadeau-lanl
Copy link
Contributor Author

It ran 19 hours ago, but github did not see the results...

@dnadeau-lanl
Copy link
Contributor Author

Ok so main is problematic and not being reported back.

@jtniehof
Copy link
Member

It ran on the branch, not on the pull request.

@jtniehof
Copy link
Member

Needs to show up here or it won't work: https://app.circleci.com/pipelines/github/spacepy/dbprocessing

@jtniehof
Copy link
Member

That actually might be the problem. If you have your fork set up to send stuff to your CircleCI account, it's probably rejecting the webhooks from the dbprocessing organization as duplicates.

@jtniehof
Copy link
Member

Check https://github.com/dnadeau-lanl/dbprocessing/settings/hooks --probably just best to remove any hooks you have there for CircleCI. That would be the difference between PRs you submit and those that other people submit.

@dnadeau-lanl
Copy link
Contributor Author

I can try that for now, but I do want to trigger circleci with my branch when I push, not only for PR.

@jtniehof
Copy link
Member

You can always open a draft PR if you want to see how things go. I just don't think it's going to work to have two sets of webhooks against the same commit.

@dnadeau-lanl dnadeau-lanl mentioned this pull request Oct 29, 2020
@jtniehof
Copy link
Member

I'm seeing a build on master; was that a manual trigger?

@dnadeau-lanl
Copy link
Contributor Author

Yes.

I don't know what is going on. It used to work....

@dnadeau-lanl
Copy link
Contributor Author

Can you submit a fake PR too. to see if you can trigger it from your github repo.

@jtniehof
Copy link
Member

Looks like the build-on-branch-and-on-PR won't work: "Currently a push to a branch may be ran before a pull request is created and a new job will not be triggered when a pull request is opened." https://ideas.circleci.com/cloud-feature-requests/p/trigger-new-build-when-a-pull-request-is-opened

Doesn't explain why we're not having stuff work now that you've disabled your webhook.

@jtniehof
Copy link
Member

Yes, I'll do some tests on my account, have another meeting now though.

@dnadeau-lanl
Copy link
Contributor Author

I found out what it was. I was following my projects in circle and the forked projects. You have to follow only the fork projects.

https://support.circleci.com/hc/en-us/articles/360008097173-Why-aren-t-pull-requests-triggering-jobs-on-my-organization-

https://app.circleci.com/projects/project-dashboard/github/spacepy

@jtniehof
Copy link
Member

The one you just pushed does, at least, link to the spacepy/dbprocessing org page on CircleCI (when I click "Details"). Looks like the postgres is still running, so hopefully life is good once it completes.

@jtniehof
Copy link
Member

Ah, I had seen that before, and of course forgot. So we should fold that into the docs, document the setting for passing the secrets through to CircleCI, and maybe do a rebase to clean up the extra commits that were just triggering, and we're good. Thanks for pushing through that.

@jtniehof
Copy link
Member

Hmmm, that did pick up a bonus commit that's just whitespace changes....

@dnadeau-lanl
Copy link
Contributor Author

dnadeau-lanl commented Oct 29, 2020 via email

@jtniehof
Copy link
Member

jtniehof commented Oct 29, 2020

I can run the documentation on this and you can check it over to see if it makes sense? (Edit, if that takes stuff off your plate, not elbowing in.)

@dnadeau-lanl
Copy link
Contributor Author

Ok thanks!

@dnadeau-lanl
Copy link
Contributor Author

I just git reset --hard and git force push on my branch. Seems to have worked.

@jtniehof
Copy link
Member

I can also do some history cleanup when I write up docs (this afternoon).

Copy link
Member

@jtniehof jtniehof left a comment

Choose a reason for hiding this comment

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

I think we're good to go here. You can't review since you opened the PR, so just post a comment and if you're good with the doc changes I'll do the merge.

@dnadeau-lanl
Copy link
Contributor Author

The docs sounds good. Something I wonder, if I push a new branch will it trigger circleci in your spacepy repo, if not I would like to find a way to make this work.

@jtniehof
Copy link
Member

Based on https://ideas.circleci.com/cloud-feature-requests/p/trigger-new-build-when-a-pull-request-is-opened , it sounds like if you push to a branch that runs a CircleCI job and then open a PR, the checks won't work on the PR.

Also, we know from here that you can't follow a fork and have the checks work in the organization's repo, and I don't think there's a way to get the organization CircleCI to run against pushes in a fork (it would be all forks, then, and that would be a mess.)

So I don't think there's a way to get the CI to run against your fork and have checks work on the pull request.

@jtniehof
Copy link
Member

jtniehof commented Nov 2, 2020

I'm going to go ahead and merge this and opened #53 to track the user CI issue.

@jtniehof jtniehof merged commit af0ab92 into spacepy:master Nov 2, 2020
Continuous Integration automation moved this from In progress to Done Nov 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or feature request testing Affects tests not functionality
Projects
Development

Successfully merging this pull request may close these issues.

Add Docker auth to CircleCI
2 participants