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.

Sign up for GitHub

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

Jump to bottom

[Resolve #1273]: Events start from response time #1275

Merged
merged 7 commits into from
Dec 14, 2022
Merged

[Resolve #1273]: Events start from response time #1275

merged 7 commits into from
Dec 14, 2022

Conversation

dboitnot
Copy link
Contributor

@dboitnot dboitnot commented Dec 11, 2022 •
edited
Loading

Resolves #1273 by starting event filtering from the timestamp returned in the AWS response headers rather than relying on the workstation clock.

PR Checklist

  • Wrote a good commit message & description [see guide below].
  • Commit message starts with [Resolve #issue-number].
  • Added/Updated unit tests.
  • Added/Updated integration tests (if applicable).
  • All unit tests (make test) are passing.
  • Used the same coding conventions as the rest of the project.
  • The new code passes pre-commit validations (pre-commit run --all-files).
  • The PR relates to only one subject with a clear title.
    and description in grammatically correct, complete sentences.

Approver/Reviewer Checklist

  • Before merge squash related commits.

Other Information

Guide to writing a good commit

@dboitnot
Copy link
Contributor Author

I'm not sure I understand yet what's going on with that unit test. As far as I know I haven't messed with the timeout behavior.

@dboitnot dboitnot mentioned this pull request Dec 11, 2022
Closed
@jfalkenstein jfalkenstein changed the title fix(gh-1273): Events start from response time [Resolve #1273]: Events start from response time Dec 12, 2022
@jfalkenstein
Copy link
Contributor

Ugh... that test that's failing is exceptionally ugly and patches things in the worst way. Feel free to rewrite it to function and read better.

jfalkenstein
jfalkenstein reviewed Dec 12, 2022
View reviewed changes
sceptre/helpers.py Outdated Show resolved Hide resolved
jfalkenstein
jfalkenstein reviewed Dec 12, 2022
View reviewed changes
sceptre/helpers.py Outdated Show resolved Hide resolved
jfalkenstein
jfalkenstein reviewed Dec 12, 2022
View reviewed changes
sceptre/helpers.py Outdated Show resolved Hide resolved
jfalkenstein
jfalkenstein reviewed Dec 12, 2022
View reviewed changes
tests/test_helpers.py Outdated Show resolved Hide resolved
@dboitnot
Copy link
Contributor Author

Just pushed commits for review which address all of the above EXCEPT the mock tests in TestStackActions. If you guys are happy with the refactor then I'll see about getting those tests back in order.

jfalkenstein
jfalkenstein reviewed Dec 13, 2022
View reviewed changes
sceptre/helpers.py Outdated Show resolved Hide resolved
jfalkenstein
jfalkenstein reviewed Dec 13, 2022
View reviewed changes
Copy link
Contributor

@jfalkenstein jfalkenstein left a comment

Choose a reason for hiding this comment

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

I like what I'm seeing. My biggest criticism is the lack of type annotations. Let's use those as much as is possible. They'll help developers as well as type-checkers prevent bugs in code in the future.

@dboitnot
Copy link
Contributor Author

Just pushed update with type annotations. If you guys are happy with where we're at, I'll dig into the mock tests.

jfalkenstein
jfalkenstein reviewed Dec 13, 2022
View reviewed changes
Copy link
Contributor

@jfalkenstein jfalkenstein 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 to me. I'm happy these changes. Let me know when you've got tests resolved.

@dboitnot
Copy link
Contributor Author

Tests passing. Note that I'm using ANY as the matcher on the calls to _wait_for_completion since a) the arg to that call isn't relevant to the tests, and b) it was unclear (to me) how to assert that the call to one mock was the mock returned by the call to the last mock.

@jfalkenstein
Copy link
Contributor

Running integration tests on this now. This generally looks pretty good to me and should be regression tested well enough by our integration tests. Assuming those pass, I'll approve.

@jfalkenstein jfalkenstein merged commit ebe7c25 into Sceptre:master Dec 14, 2022
@dboitnot dboitnot deleted the gh-1273 branch December 14, 2022 19:52
jfalkenstein added a commit that referenced this pull request Feb 11, 2023
@jfalkenstein
Releasing v4.0.0 (#1292)
b8d6319 
## 4.0.0 (2023.02.08)

### Added
 - [Resolve #1283] Introducing `sceptre_role`, `cloudformation_service_role` (#1295)
   - These are just iam_role and role_arn renamed to be a lot clearer. See "Deprecations" below.


### Changed
 - [Resolve #1299] Making the ConnectionManager a more "friendly" interface for hooks, resolvers,
   and template handlers (#1287, #1300)
   - This creates adds the public `get_session()` and
     `create_session_environment_variables()` methods to make AWS interactions
     easier and more consistent with individual stack configurations for
     iam_role, profile, and region configurations.
   - The `call()` method now properly distinguishes between default stack
     configurations for profile, region, and `sceptre_role` and setting those to
     `None` to nullify them.
 - Preventing Duplicate Deprecation Warnings from being emitted (#1297)

#### _Potentially_ Breaking Changes
 - The !cmd hook now invokes the passed command using the AWS environment
   variables that correspond with the stack's IAM configurations (i.e. iam_role,
   profile, region). This means that the hook will operate the same as every
   other part of Sceptre and regard how the stack is configured. This should make
   it easier to invoke other tools like AWS CLI with your hooks. However, if
   your project is setting environment variables with the intent to change how
   the command authenticates with AWS (such as a different token, profile, or
   region), these environment variables will be overridden. To maintain the same
   functionality, you should prefix your command with
   `export AWS_SESSION_TOKEN={{environment_variable.AWS_SESSION_TOKEN}} &&` (or
   whatever other environment variable(s) you need to explicitly set).

### Deprecations
 - [Resolve #1283] Deprecating `iam_role`, `role_arn`, and `template_path` (#1295)
   - `iam_role` and `role_arn` have been aliased to `sceptre_role` and
      `cloudformation_service_role`. Using these fields will result in a
      DeprecationWarning.
   - `template_path` has actually been slated for removal since v2.7. `template`
      should be used instead. Using `template_path` will result in a
      DeprecationWarning.
   - All three deprecated StackConfig fields will be removed in v5.0.0.

## 3.3.0 (2023.02.06)

### Added
 - [Resolve #1261] Add coloured differ (#1260)
   - Implements coloured diffs for the diff (difflib) command. Responds to --no-color.
 - [Resolves #1271] Extend stack colourer to include "IMPORT" states (#1272)
 - [Resolves #1179] cloudformation disable-rollback option (#1282)
   - Allow user to disable a cloudformation rollback on a sceptre deployment.

### Changed
 - [Resolve #1098] Deploy docker container to sceptreorg repo (#1265)
   - Deploy sceptre docker images to dockerhub sceptreorg repo instead of cloudreach repo
 - Updating Setuptools and wheel versions to avert security issues
 - [Resolve #1293] Improve the Stack Config Jinja Syntax Error Message to include the Stack Name (#1294)
 - [Resolves #1267] Improve the Stack Config Jinja Error Message to include the Stack Name (#1269)

### Fixed
 - [Resolve #1273] Events start from response time (#1275)
   - Resolves #1273 by starting event filtering from the timestamp returned in
     the AWS response headers rather than relying on the workstation clock.
 - [Resolve #1253] Failed downloads raise error (#1277)
   - Throwing an informative error when the template fails to download instead
     of passing the error message to CloudFormation.
 - [Resolves #1179] Changed disable-rollback default to None (#1288)
   - We want the default value to be None to represent "Do whatever's configured
     in the StackConfig" and True/False will override the StackConfig.

### Nonfunctional
 - Add tweet-release to CircleCI config (#1259)
 - [Resolves #1276] Adopt Black Auto-formatter (#1278)
   - Reformatting all Python files using the Black code formatter. This also
     delivers a new function for generating `__repr__` methods which was needed
     to deal with a line-too-long issue in Template. Per discussion in #1276 this
     PR also disables E203 in flake8.
 - Update sceptre-circleci docker image (#1284)
   - Update to build and test with a docker image that's based on the official
     circleci python docker image.
 - [Resolve #1264] Updating the CDK docs to point to the new sceptre-cdk-handler
   (#1285)
   - This updates our docs to no longer reference the old CDK approach (which
     didn't work with CDK assets). In its place, it references the new
     sceptre-cdk-handler package that covers that functionality.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jfalkenstein jfalkenstein jfalkenstein approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Events missing from launch output
2 participants
@dboitnot @jfalkenstein