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

DYN-5755 : restrict logging in service mode #13860

Merged
merged 2 commits into from
Apr 4, 2023

Conversation

BogdanZavu
Copy link
Contributor

@BogdanZavu BogdanZavu commented Mar 30, 2023

Purpose

We have to deal with some limitations when we deploy Dynamo in service mode.
More precisely there are restrictions when running Dynamo in a Lambda and we can't create local files.

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated


notifications = new List<NotificationMessage>();

testMode = isTestMode;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we should merge all these flags somehow at some point

Copy link
Member

@mjkkirschner mjkkirschner Mar 31, 2023

Choose a reason for hiding this comment

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

If you throw in an obsolete message and a TODO + Dynamo 3.0 it will be easier to find later when we want to refactor all these signatures throughout the code base.

@mjkkirschner
Copy link
Member

mjkkirschner commented Mar 31, 2023

hey @BogdanZavu there is one unusual failing test:
https://master-5.jenkins.autodesk.com/job/Dynamo/job/DynamoSelfServe/job/pullRequestValidation/8781/testReport/junit/Dynamo.Tests.Search/SearchDictionaryTest/TestSearchDictionaryPerformance/

can you try it locally?

Unfortunately the build machines seem to be down so I can't kick off another rebuild to see if it's intermittent, I will look into that - also will review.

@@ -2418,6 +2418,12 @@ private void RegisterHomeWorkspace(HomeWorkspaceModel newWorkspace)
/// </summary>
protected void SaveBackupFiles(object state)
{
//No backup files in ServiceMode due to Lambda restrictions
if (IsServiceMode)
Copy link
Member

Choose a reason for hiding this comment

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

would be nice to have a test for this if there are already backup tests

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we do

@mjkkirschner
Copy link
Member

passed here:
https://master-5.jenkins.autodesk.com/job/Dynamo/job/DynamoSelfServe/job/pullRequestValidation/8786/

Copy link
Member

@mjkkirschner mjkkirschner 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 but a few comments

@mjkkirschner mjkkirschner merged commit f657677 into DynamoDS:master Apr 4, 2023
sm6srw pushed a commit that referenced this pull request Apr 5, 2023
* restrict logging in service mode

* add todo

---------

Co-authored-by: Bogdan Zavu <[email protected]>

passed.
@sm6srw sm6srw mentioned this pull request Apr 11, 2023
9 tasks
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.

4 participants