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

fix: delete old instances then create new instance #955

Merged
merged 2 commits into from
May 8, 2020

Conversation

skuruppu
Copy link
Contributor

@skuruppu skuruppu commented May 7, 2020

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #954 🦕

@skuruppu skuruppu added the api: spanner Issues related to the googleapis/nodejs-spanner API. label May 7, 2020
@skuruppu skuruppu requested a review from hengfengli May 7, 2020 00:49
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 7, 2020
@skuruppu
Copy link
Contributor Author

skuruppu commented May 7, 2020

Totally not tested yet, let's see if this runs.

@hengfengli
Copy link
Contributor

I don't think this would work. INSTANCE_ID is generated by date and I guess yesterday's instance was not deleted. All the deleting instances calls use the instance ID based on today not yesterday or past days.

const INSTANCE_ID = process.env.SPANNERTEST_INSTANCE || `test-instance-${date}`;
const INSTANCE_ALREADY_EXISTS = !!process.env.SPANNERTEST_INSTANCE;

@skuruppu
Copy link
Contributor Author

skuruppu commented May 7, 2020

It worked! Except for the fact that a backup couldn't be completed within the test timeout period :(

Copy link
Contributor

@hengfengli hengfengli left a comment

Choose a reason for hiding this comment

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

I see. It deletes all outdated instances first. It makes sense. I misunderstood how the deletion of instances work. I thought it only delete the current testing instance. LGTM.

@skuruppu skuruppu added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 7, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 7, 2020
@codecov
Copy link

codecov bot commented May 8, 2020

Codecov Report

Merging #955 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #955   +/-   ##
=======================================
  Coverage   98.20%   98.20%           
=======================================
  Files          21       21           
  Lines       19959    19959           
  Branches     1069     1069           
=======================================
  Hits        19601    19601           
  Misses        355      355           
  Partials        3        3           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0854c70...4fc2f79. Read the comment docs.

@skuruppu skuruppu merged commit 96813f8 into master May 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the googleapis/nodejs-spanner API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

samples tests can't run due to old instances not being cleaned up
4 participants