Skip to content

add acm cert bundle#130

Closed
rhockenbury wants to merge 3 commits into
sqlalchemy-redshift:masterfrom
rhockenbury:master
Closed

add acm cert bundle#130
rhockenbury wants to merge 3 commits into
sqlalchemy-redshift:masterfrom
rhockenbury:master

Conversation

@rhockenbury

@rhockenbury rhockenbury commented Sep 20, 2017

Copy link
Copy Markdown

Todos

  • MIT compatible
  • Tests
  • Documentation
  • Updated CHANGES.rst

@jklukas

jklukas commented Sep 20, 2017

Copy link
Copy Markdown
Member

I'll pull this down and run tests against the cluster.

@graingert graingert left a comment

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.

You need to update the manifest etc

@rhockenbury

Copy link
Copy Markdown
Author

@graingert

Updated.

@jklukas

jklukas commented Sep 20, 2017

Copy link
Copy Markdown
Member

I'm seeing errors when running the tests against Redshift which look entirely unrelated:

E       assert 'CREATE TABLE...ISTSTYLE EVEN' == 'CREATE TABLE ...ISTSTYLE EVEN'
E         Skipping 89 identical leading characters in diff, use -v to show
E         -  INTEGER, CONSTRAINT reflection_composite_fk_constraint_pkey PRIMARY KEY (id), CONSTRAINT reflection_composite_fk_constraint_col1_fkey FOREIGN KEY(col1, col2) REFERENCES reflection_pk_constraint (col1, col2) ) DISTSTYLE EVEN
E         +  INTEGER, PRIMARY KEY (id), FOREIGN KEY(col1, col2) REFERENCES reflection_pk_constraint (col1, col2) ) DISTSTYLE EVEN

Could this be a regression from a recent change in this repo, or does this look like a change in Redshift's behavior? It looks like constraints are being returned differently.

@jklukas

jklukas commented Sep 20, 2017

Copy link
Copy Markdown
Member

Travis is showing InterpreterNotFound errors, which I've also not seen.

@rhockenbury

Copy link
Copy Markdown
Author

Looking at the travis logs, I'm seeing:

E: Failed to fetch http://dl.hhvm.com/ubuntu/dists/trusty/main/binary-i386/Packages 503 Service Unavailable [IP: 140.211.166.134 80]
E: Some index files failed to download. They have been ignored, or old ones used instead.

@jklukas

jklukas commented Sep 20, 2017

Copy link
Copy Markdown
Member

Looks like the travis failures are recovering. I kicked off restarts of the last 2 failing test scenarios.

@jklukas

jklukas commented Sep 20, 2017

Copy link
Copy Markdown
Member

I'm getting the errors discussed in #130 (comment) when running master branch tests against Redshift, so I think something changed with Redshift itself that will need to be fixed separately from this issue.

@mjschultz

Copy link
Copy Markdown
Contributor

It would be nice to verify the new bundle against the new certs AWS will be using prior to them actually forcing everyone to changeover. From the email and page when I read it that doesn't seem possible right now (which scares me).

I can open a support issue inquiring about that ability unless you have that ability?

@jklukas

jklukas commented Sep 20, 2017

Copy link
Copy Markdown
Member

I'd love if you go ahead and open the support case, @mjschultz

@jklukas jklukas left a comment

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.

To my knowledge, all test failures actually related to this change are now resolved.

@jleclanche jleclanche mentioned this pull request Sep 23, 2017
4 tasks
@jleclanche

Copy link
Copy Markdown
Contributor

I've filed an alternative pr in #132 which cleans up the issues here and some other things.

@graingert

Copy link
Copy Markdown
Member

closing in favor of #132

@graingert graingert closed this Sep 25, 2017
graingert pushed a commit that referenced this pull request Sep 25, 2017
* Move redshift-ssl-ca-cert.pem -> redshift-ca-bundle.crt

* Use certificate directly from the package in test_default_ssl

* Update redshift certificate to transitional bundle

Closes #129
Closes #130
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.

5 participants