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

feat: support yml anchors during push command #903

Merged
merged 2 commits into from Mar 14, 2024

Conversation

vigneshshanmugam
Copy link
Member

devcorpio
devcorpio previously approved these changes Mar 13, 2024
Copy link
Contributor

@devcorpio devcorpio left a comment

Choose a reason for hiding this comment

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

code LGTM

Edit: I will test it manually and let you know

@devcorpio
Copy link
Contributor

devcorpio commented Mar 14, 2024

TL;DR: testing panned out 🚀, so LGTM 🎉

btw, at the end you will see an observations section I added, let me know what you think:

Details:

YML used:

http-type: &http-type
  type: http
schedule5: &schedule-1
  schedule: '@every 1m'
team-tag: &team-tag
  tags:
    - team1
    - team2

heartbeat.monitors:
- id: http1
  name: "http1"
  schedule: '@every 1m'
  <<: *http-type
  <<: *team-tag
  urls: "https://elastic.github.io/synthetics-demo/"
- id: http2
  name: "http2"
  <<: *http-type
  <<: *schedule-1
  urls: "https://elastic.co"
- id: tcp1
  name: "tcp1"
  type: tcp
  <<: *schedule-1
  hosts: ["www.elastic.co:443"]

case 1: results of using the YML anchor in a branch without the new code changes

Screenshot 2024-03-14 at 12 08 32

I tested out of curiosity. Above, you can see what the CLI displayed.

The interesting thing is that in Kibana I was able to see two monitors (although they have been created as journeys rather than http), see below:

Screenshot 2024-03-14 at 12 25 54

case 2: results of using the YML anchor in the PR's branch

since http1 and http2 were created as journeys in the previous step. Now we can see the following when executing the command:

Screenshot 2024-03-14 at 12 28 25

This is a good signal since that tells us the new logic is involved 🚀

Now, after removing all monitors and starting afresh, the CLI shows this:

Screenshot 2024-03-14 at 12 31 18

Besides, on this Kibana page we can see clearly how things have been set as expected:

Screenshot 2024-03-14 at 12 24 02

I also tested pushing the same YML again (without any changes):

Screenshot 2024-03-14 at 12 34 09

all good, too.

I also tested pushing the YML with a little variation (changing the URL of the TCP monitor):

Screenshot 2024-03-14 at 12 35 22

everything is okay as well.

OBSERVATIONS

1. Perhaps not super important, but when writing the YML, VS code was complaining about duplicated keys:

Screenshot 2024-03-14 at 12 39 27

Display related to:

Screenshot 2024-03-14 at 12 39 58

Should we document this or mention it somehow? Maybe it's not important I'm not sure, but wanted to highlight it. It doesn't affect the functionality at all.

2. Uploading a TCP monitor with a host that is "malformed" such as "https://elastic.co" (it should be elastic.co:443) makes the monitor fail, which makes sense, but what caught my attention is that in the UI it says that the URL is unavailable:

Screenshot 2024-03-14 at 12 45 22

which seems weird, at the beginning I thought that the YML wasn't working and that the URL was empty.

But after updating to the proper value (elastic.co:443), I started seeing the URL set again:

Screenshot 2024-03-14 at 12 49 32

again, just something that wanted to highlight, perhaps is an intended behaviour

3. I will update the unit test, although it works, I believe that we should use all the required fields for each monitor type. For instance, urls and hosts, saying mainly because tests can also be a good place where to extract examples from, just a nitpick thingy 😄

edit: commit

edit2: oh no, now my approval is useless because I pushed code, @emilioalvap will need your review too 🚀

@vigneshshanmugam
Copy link
Member Author

@devcorpio Thanks for the through tests 🎉

Regarding the VScode thing, we really don't control that and its the YML formatting/extension that throws these lint errors. We can discard them as its up to the library to handle these.

Copy link
Contributor

@emilioalvap emilioalvap left a comment

Choose a reason for hiding this comment

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

LGTM

@vigneshshanmugam vigneshshanmugam merged commit 63e5e4a into elastic:main Mar 14, 2024
7 checks passed
@vigneshshanmugam vigneshshanmugam deleted the anchor-yml-support branch March 14, 2024 18:44
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.

yaml anchor support for lightweight monitors
3 participants