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

Replace AWSCore with AWS.jl #28

Closed
wants to merge 4 commits into from
Closed

Replace AWSCore with AWS.jl #28

wants to merge 4 commits into from

Conversation

ararslan
Copy link
Collaborator

Fixes #27.

Ping @mattBrzezinski for review. 🙂

@ararslan
Copy link
Collaborator Author

Is there no CI on this repo?

@mattBrzezinski
Copy link
Member

Looks like #26 never got merged, I pinged Fernando around it. Should also move this into the JuliaCloud org too.

Copy link
Member

@mattBrzezinski mattBrzezinski left a comment

Choose a reason for hiding this comment

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

LGTM, just want to get #26 in before hand so we can test that the changes work.

test/runtests.jl Outdated Show resolved Hide resolved
Project.toml Outdated Show resolved Hide resolved
src/CloudWatchLogs.jl Outdated Show resolved Hide resolved
src/stream.jl Outdated Show resolved Hide resolved
@ararslan
Copy link
Collaborator Author

@mattBrzezinski, it might be good to have you re-review if you get the time, since the refactoring to remove the internal function that mimics the old AWSCore API is a larger change introduced since your approval.

Copy link
Member

@mattBrzezinski mattBrzezinski 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 LGTM! Thanks for simplified the test files too :)
Just need to get around to #26 before this will go in. It just seems to be a timing issue like almost all the AWS CI errors happen to be. Might just merge that in anyways.

@mattBrzezinski
Copy link
Member

Oh this is awkward... I don't remember how GitHub handles the introduction of CI workflows for merge requests that exist before those going in.

I believe a rebase onto master should resolve this and kick it off? Do you mind trying that out @ararslan , otherwise we may need to close and re-open a PR off of this branch.

@ararslan
Copy link
Collaborator Author

I rebased on master but it looks like running CI requires approval by someone with commit access.

@ararslan
Copy link
Collaborator Author

Sigh, needs another approval. Sorry 😬

@mattBrzezinski
Copy link
Member

Sigh, needs another approval. Sorry 😬

No worries, kind of silly it needs to be done after I approved it once before, oh well.

@ararslan
Copy link
Collaborator Author

Hm. Seems something still isn't quite right. 🤔

@iamed2
Copy link
Member

iamed2 commented May 27, 2021

Could this be failing because this doesn't have bors, so it's running without access to our secrets since this is a PR from a fork?

@iamed2
Copy link
Member

iamed2 commented May 27, 2021

@ararslan I've invited you as a collaborator with Write access so you could re-push this PR on the repo itself.

@ararslan
Copy link
Collaborator Author

Quite possibly! Thanks for the invite, I'll close this and reopen from a branch.

@ararslan ararslan closed this May 27, 2021
@mattBrzezinski
Copy link
Member

Could this be failing because this doesn't have bors, so it's running without access to our secrets since this is a PR from a fork?

This might be an issue w/ the high-level AWS functions. I'm looking into it now.

@ararslan ararslan deleted the aa/aws-jl branch May 27, 2021 19:46
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.

does not play nicely with AWS.jl
3 participants